[PATCH 0/1] scripts/package/builddeb: allow hooks also in /usr/share/kernel

[PATCH 0/1] scripts/package/builddeb: allow hooks also in /usr/share/kernel

[Johannes Schauer Marin Rodrigues]

Hi,

TLDR: allow packages in Debian and derivatives to install kernel hooks into
/usr/share/kernel instead of using /etc/kernel. In a nutshell, it calls
run-parts like this:

    run-parts /etc/kernel/postinst.d /usr/lib/kernel/postinst.d

When Debian packages install files (including kernel maintainer script hooks)
into /etc, they will be marked as a conffile. This has undesirable side-effects
like the file will not be removed upon package removal (unless purged) and if
the user changes the file, then deploying fixes is not straight forward
anymore. This patch allows distributions to to install their kernel hooks into
/usr/share/kernel from where they are then picked up by run-parts. If the admin
wants to either disable or change the hooks, they can do so by placing a file
into /etc/kernel. Files placed in /etc/kernel will override files of the same
name in /usr/share/kernel. This mechanism might be known from how systemd and
related software organizes configuration files in /etc and /usr. The mechanism
is documented in as the UAPI Configuration Files Specification:
https://uapi-group.org/specifications/specs/configuration_files_specification/

This functionality relies on run-parts 5.21 or later. If run-parts is lacking
support, only scripts in /etc/kernel will be considered. It is the
responsibility of packages installing hooks into /usr/share/kernel to also
declare a Depends: debianutils (>= 5.21). The logic I implemented tries hard to
not require this new functionality by checking whether either of the
directories exists and is empty or not. Only when both directories exist and
contain files is run-parts executed with both directories as arguments. Since
this will fail if run-parts is too old, a check is added to test the run-parts
version. Unfortunately the run-parts --version output is not quite machine
readable, so I agreed with the debianutils maintainer to use the --help output
instead. Should run-parts be too old, then only the /etc directory is passed
to run-parts and thus files in /usr will be ignored. It is the responsibility
of distribution packages to declare a dependency on debianutils (>= 5.21) once
they start placing files into /usr/share/kernel.

The if/else tree can be considerably simplified, once this new functionality
of run-parts 5.21 has been in a few Debian stable releases. Until then, I think
it makes sense to try and be conservative and try to execute run-parts with
only a single directory if possible.

What do you think?

Thanks!

cheers, josch



Johannes Schauer Marin Rodrigues (1):
  scripts/package/builddeb: allow hooks also in /usr/share/kernel

 scripts/package/builddeb | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

-- 
2.39.2

[PATCH 1/1] scripts/package/builddeb: allow hooks also in /usr/share/kernel

[Johannes Schauer Marin Rodrigues]

By passing an additional directory to run-parts, allow Debian and its
derivatives an easy way to ship maintainer scripts in /usr while at the
same time allowing the local admin to easily override or disable them by
placing hooks of the same name in /etc. This adds support for the
mechanism described in the UAPI Configuration Files Specification for
kernel hooks:
https://uapi-group.org/specifications/specs/configuration_files_specification/

This functionality relies on run-parts 5.21 or later. If run-parts is
lacking support, only scripts in /etc/kernel will be considered. It is
the responsibility of packages installing hooks into /usr/share/kernel
to also declare a Depends: debianutils (>= 5.21).
---
 scripts/package/builddeb | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/scripts/package/builddeb b/scripts/package/builddeb
index 441b0bb66e0d..1fa5228eed0b 100755
--- a/scripts/package/builddeb
+++ b/scripts/package/builddeb
@@ -5,10 +5,14 @@
 #
 # Simple script to generate a deb package for a Linux kernel. All the
 # complexity of what to do with a kernel after it is installed or removed
-# is left to other scripts and packages: they can install scripts in the
-# /etc/kernel/{pre,post}{inst,rm}.d/ directories (or an alternative location
-# specified in KDEB_HOOKDIR) that will be called on package install and
-# removal.
+# is left to other scripts and packages. Scripts can be placed into
+# the preinst, postinst, prerm and postrm directories in /etc/kernel
+# (override with KDEB_HOOKDIR) or /usr/share/kernel (override with
+# KDEB_DISTRO_HOOKDIR). Hooks of the same name in KDEB_HOOKDIR will override
+# hooks in KDEB_DISTRO_HOOKDIR. This, the former directory (usually in /etc)
+# can be used by the local admin while the latter directory (usually in /usr)
+# can be used by packages shipped by the distribution. The preinst, postinst,
+# prerm and postrm will then be called on package installation and removal.
 
 set -eu
 
@@ -69,8 +73,10 @@ install_linux_image () {
 	# make-kpkg sets $INITRD to indicate whether an initramfs is wanted, and
 	# so do we; recent versions of dracut and initramfs-tools will obey this.
 	debhookdir=${KDEB_HOOKDIR:-/etc/kernel}
+	debdistrohookdir=${KDEB_DISTRO_HOOKDIR:-/usr/share/kernel}
 	for script in postinst postrm preinst prerm; do
 		mkdir -p "${pdir}${debhookdir}/${script}.d"
+		mkdir -p "${pdir}${debdistrohookdir}/${script}.d"
 
 		mkdir -p "${pdir}/DEBIAN"
 		cat <<-EOF > "${pdir}/DEBIAN/${script}"
@@ -84,7 +90,30 @@ install_linux_image () {
 		# Tell initramfs builder whether it's wanted
 		export INITRD=$(if_enabled_echo CONFIG_BLK_DEV_INITRD Yes No)
 
-		test -d ${debhookdir}/${script}.d && run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" ${debhookdir}/${script}.d
+		if test -d "${debhookdir}/${script}.d" && test -d "${debdistrohookdir}/${script}.d"; then
+			if test -n "$(find "${debhookdir}/${script}.d" -maxdepth 0 -empty)" ; then
+				# KDEB_HOOKDIR is empty, execute run-parts on KDEB_DISTRO_HOOKDIR only
+				run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" "${debdistrohookdir}/${script}.d"
+			elif test -n "$(find "${debdistrohookdir}/${script}.d" -maxdepth 0 -empty)" ; then
+				# KDEB_DISTROHOOKDIR is empty, execute run-parts on KDEB_HOOKDIR only
+				run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" "${debhookdir}/${script}.d"
+			else
+				# Both KDEB_HOOKDIR and KDEB_DISTROHOOKDIR contain files. It is the
+				# responsibility of the distribution package that placed files into
+				# KDEB_DISTROHOOKDIR to add a Depends: debianutils (>= 5.21)
+				if run-parts --help 2>&1 | grep -Fxq "Usage: run-parts [OPTION]... DIRECTORY [DIRECTORY ...]"; then
+					run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" "${debhookdir}/${script}.d" "${debdistrohookdir}/${script}.d"
+				else
+					echo "E: Ignoring maintainer scripts in ${debdistrohookdir} because run-parts is too old (5.21 required)" >&2
+					run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" "${debhookdir}/${script}.d"
+				fi
+			fi
+		elif test -d "${debhookdir}/${script}.d"; then
+			run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" "${debhookdir}/${script}.d"
+		elif test -d "${debdistrohookdir}/${script}.d"; then
+			run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" "${debdistrohookdir}/${script}.d"
+		fi
+
 		exit 0
 		EOF
 		chmod 755 "${pdir}/DEBIAN/${script}"
-- 
2.39.2

Re: [PATCH 1/1] scripts/package/builddeb: allow hooks also in /usr/share/kernel

[Masahiro Yamada]

On Wed, Nov 27, 2024 at 7:59 PM Johannes Schauer Marin Rodrigues
<josch@mister-muffin.de> wrote:
>
> By passing an additional directory to run-parts, allow Debian and its
> derivatives an easy way to ship maintainer scripts in /usr while at the
> same time allowing the local admin to easily override or disable them by
> placing hooks of the same name in /etc. This adds support for the
> mechanism described in the UAPI Configuration Files Specification for
> kernel hooks:
> https://uapi-group.org/specifications/specs/configuration_files_specification/
>
> This functionality relies on run-parts 5.21 or later. If run-parts is
> lacking support, only scripts in /etc/kernel will be considered. It is
> the responsibility of packages installing hooks into /usr/share/kernel
> to also declare a Depends: debianutils (>= 5.21).


Signed-off-by: is required for any kernel patch.



> ---
>  scripts/package/builddeb | 39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/package/builddeb b/scripts/package/builddeb
> index 441b0bb66e0d..1fa5228eed0b 100755
> --- a/scripts/package/builddeb
> +++ b/scripts/package/builddeb
> @@ -5,10 +5,14 @@
>  #
>  # Simple script to generate a deb package for a Linux kernel. All the
>  # complexity of what to do with a kernel after it is installed or removed
> -# is left to other scripts and packages: they can install scripts in the
> -# /etc/kernel/{pre,post}{inst,rm}.d/ directories (or an alternative location
> -# specified in KDEB_HOOKDIR) that will be called on package install and
> -# removal.
> +# is left to other scripts and packages. Scripts can be placed into
> +# the preinst, postinst, prerm and postrm directories in /etc/kernel
> +# (override with KDEB_HOOKDIR) or /usr/share/kernel (override with
> +# KDEB_DISTRO_HOOKDIR). Hooks of the same name in KDEB_HOOKDIR will override
> +# hooks in KDEB_DISTRO_HOOKDIR. This, the former directory (usually in /etc)
> +# can be used by the local admin while the latter directory (usually in /usr)
> +# can be used by packages shipped by the distribution. The preinst, postinst,
> +# prerm and postrm will then be called on package installation and removal.
>
>  set -eu
>
> @@ -69,8 +73,10 @@ install_linux_image () {
>         # make-kpkg sets $INITRD to indicate whether an initramfs is wanted, and
>         # so do we; recent versions of dracut and initramfs-tools will obey this.
>         debhookdir=${KDEB_HOOKDIR:-/etc/kernel}
> +       debdistrohookdir=${KDEB_DISTRO_HOOKDIR:-/usr/share/kernel}
>         for script in postinst postrm preinst prerm; do
>                 mkdir -p "${pdir}${debhookdir}/${script}.d"
> +               mkdir -p "${pdir}${debdistrohookdir}/${script}.d"
>
>                 mkdir -p "${pdir}/DEBIAN"
>                 cat <<-EOF > "${pdir}/DEBIAN/${script}"
> @@ -84,7 +90,30 @@ install_linux_image () {
>                 # Tell initramfs builder whether it's wanted
>                 export INITRD=$(if_enabled_echo CONFIG_BLK_DEV_INITRD Yes No)
>
> -               test -d ${debhookdir}/${script}.d && run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" ${debhookdir}/${script}.d
> +               if test -d "${debhookdir}/${script}.d" && test -d "${debdistrohookdir}/${script}.d"; then
> +                       if test -n "$(find "${debhookdir}/${script}.d" -maxdepth 0 -empty)" ; then


This must be escaped like "\$(find ...)"
because the 'find' command must be evaluated on the
installed system instead of on the build machine.

However, the added complexities look skeptical to me.

If /usr/share/kernel/postinst.d/ exists on the installed system,
we can assume run-parts>=5.21 exists there.


One more thing, do we need a new env variable
for the second search path?

If a user wants to add more search paths for custom hooks,
they can pass multiple words to KDEB_HOOKDIR.

export KDEB_HOOKDIR="
/home/masahiro/etc/kernel
/etc/kernel
/usr/local/share/kernel
/usr/share/kernel
"


This is compatible with the current "single path for KDEB_HOOKDIR"
usage, and flexible enough for the new extension.








> +                               # KDEB_HOOKDIR is empty, execute run-parts on KDEB_DISTRO_HOOKDIR only
> +                               run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" "${debdistrohookdir}/${script}.d"
> +                       elif test -n "$(find "${debdistrohookdir}/${script}.d" -maxdepth 0 -empty)" ; then
> +                               # KDEB_DISTROHOOKDIR is empty, execute run-parts on KDEB_HOOKDIR only
> +                               run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" "${debhookdir}/${script}.d"
> +                       else
> +                               # Both KDEB_HOOKDIR and KDEB_DISTROHOOKDIR contain files. It is the
> +                               # responsibility of the distribution package that placed files into
> +                               # KDEB_DISTROHOOKDIR to add a Depends: debianutils (>= 5.21)
> +                               if run-parts --help 2>&1 | grep -Fxq "Usage: run-parts [OPTION]... DIRECTORY [DIRECTORY ...]"; then
> +                                       run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" "${debhookdir}/${script}.d" "${debdistrohookdir}/${script}.d"
> +                               else
> +                                       echo "E: Ignoring maintainer scripts in ${debdistrohookdir} because run-parts is too old (5.21 required)" >&2
> +                                       run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" "${debhookdir}/${script}.d"
> +                               fi
> +                       fi
> +               elif test -d "${debhookdir}/${script}.d"; then
> +                       run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" "${debhookdir}/${script}.d"
> +               elif test -d "${debdistrohookdir}/${script}.d"; then
> +                       run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" "${debdistrohookdir}/${script}.d"
> +               fi
> +
>                 exit 0
>                 EOF
>                 chmod 755 "${pdir}/DEBIAN/${script}"
> --
> 2.39.2
>
>


-- 
Best Regards
Masahiro Yamada

[PATCH v2 0/1] scripts/package/builddeb: allow hooks also in /usr/share/kernel

[Johannes Schauer Marin Rodrigues]

Hi,

Quoting Masahiro Yamada (2024-11-28 02:38:40)
> Signed-off-by: is required for any kernel patch.

whoops, you see I'm not doing this often. :D Thank you for your
patience.

> However, the added complexities look skeptical to me.

Yes, I struggle to find a good balance between the problem that only recent
versions of run-parts will support this versus trying to be safe and doing the
best we can if things go badly. I think this new version is quite a bit easier
to read.

> If /usr/share/kernel/postinst.d/ exists on the installed system,
> we can assume run-parts>=5.21 exists there.

But what shall we do if it does not? My new patch falls back to only calling
run-parts on the first directory (which will be /etc by default) in an attempt
to not leave the user hanging. The assumption here is, that distros will
probably want to only allow packages placing hooks into /usr once a version
of debianutils with support for this made it into a stable release. This is
still a while off. This means, that for a long while, all the important scripts
will still all be in the familiar path in /etc.

> One more thing, do we need a new env variable
> for the second search path?
>█
> If a user wants to add more search paths for custom hooks,
> they can pass multiple words to KDEB_HOOKDIR.
>█
> export KDEB_HOOKDIR="
> /home/masahiro/etc/kernel
> /etc/kernel
> /usr/local/share/kernel
> /usr/share/kernel
> "
>█
> This is compatible with the current "single path for KDEB_HOOKDIR"
> usage, and flexible enough for the new extension.

Okay. Indeed adding the home directory or /usr/local sounds like valid
use-cases to me. I am a bit hesitant because this forces the paths to
not contain spaces but I guess that's a reasonable requirement to make.

Thank you for your review!

cheers, josch

[PATCH 1/1] scripts/package/builddeb: allow hooks also in /usr/share/kernel

[Johannes Schauer Marin Rodrigues]

By passing an additional directory to run-parts, allow Debian and its
derivatives an easy way to ship maintainer scripts in /usr while at the
same time allowing the local admin to easily override or disable them by
placing hooks of the same name in /etc. This adds support for the
mechanism described in the UAPI Configuration Files Specification for
kernel hooks:
https://uapi-group.org/specifications/specs/configuration_files_specification/

This functionality relies on run-parts 5.21 or later.  It is the
responsibility of packages installing hooks into /usr/share/kernel to
also declare a Depends: debianutils (>= 5.21).

KDEB_HOOKDIR can be used to change the list of directories that is
searched. By default, /etc/kernel and /usr/share/kernel are hook
directories.

Signed-off-by: Johannes Schauer Marin Rodrigues <josch@mister-muffin.de>
---
 scripts/package/builddeb | 44 ++++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/scripts/package/builddeb b/scripts/package/builddeb
index 441b0bb66e0d..2772146a76ce 100755
--- a/scripts/package/builddeb
+++ b/scripts/package/builddeb
@@ -5,10 +5,12 @@
 #
 # Simple script to generate a deb package for a Linux kernel. All the
 # complexity of what to do with a kernel after it is installed or removed
-# is left to other scripts and packages: they can install scripts in the
-# /etc/kernel/{pre,post}{inst,rm}.d/ directories (or an alternative location
-# specified in KDEB_HOOKDIR) that will be called on package install and
-# removal.
+# is left to other scripts and packages. Scripts can be placed into the
+# preinst, postinst, prerm and postrm directories in /etc/kernel or
+# /usr/share/kernel. A different list of search directories can be given
+# via KDEB_HOOKDIR. Scripts in directories earlier in the list will
+# override scripts of the same name in later directories.  The script will
+# be called on package installation and removal.
 
 set -eu
 
@@ -68,11 +70,18 @@ install_linux_image () {
 	# kernel packages, as well as kernel packages built using make-kpkg.
 	# make-kpkg sets $INITRD to indicate whether an initramfs is wanted, and
 	# so do we; recent versions of dracut and initramfs-tools will obey this.
-	debhookdir=${KDEB_HOOKDIR:-/etc/kernel}
+	debhookdir=${KDEB_HOOKDIR:-/etc/kernel /usr/share/kernel}
+
+	# Only pre-create the first hook directory. Support for more than one hook
+	# directory requires run-parts 5.21 and it is the responsibility of packages
+	# creating additional hook directories to declare that dependency.
+	firsthookdir=${debhookdir%% *}
 	for script in postinst postrm preinst prerm; do
-		mkdir -p "${pdir}${debhookdir}/${script}.d"
+		mkdir -p "${pdir}${firsthookdir}/${script}.d"
+	done
 
-		mkdir -p "${pdir}/DEBIAN"
+	mkdir -p "${pdir}/DEBIAN"
+	for script in postinst postrm preinst prerm; do
 		cat <<-EOF > "${pdir}/DEBIAN/${script}"
 		#!/bin/sh
 
@@ -84,7 +93,26 @@ install_linux_image () {
 		# Tell initramfs builder whether it's wanted
 		export INITRD=$(if_enabled_echo CONFIG_BLK_DEV_INITRD Yes No)
 
-		test -d ${debhookdir}/${script}.d && run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" ${debhookdir}/${script}.d
+		# run-parts will error out if one of its directory arguments does not
+		# exist, so filter the list of hook directories accordingly.
+		hookdirs=
+		for dir in ${debhookdir}; do
+			test -d "\$dir/${script}.d" || continue
+			hookdirs="\$hookdirs \$dir/${script}.d"
+		done
+		hookdirs="\${hookdirs# }"
+		test -n "\$hookdirs" || exit 0
+
+		# If more than one hook directory remained, check version of run-parts. If
+		# run-parts is too old, fall back to only processing the first.
+		case \$hookdirs in *" "*) if ! run-parts --help 2>&1 \
+				| grep -Fxq "Usage: run-parts [OPTION]... DIRECTORY [DIRECTORY ...]"; then
+				echo "E: run-parts >=5.21 is required for multiple hook directories, falling back to $firsthookdir" >&2
+				test -d "${firsthookdir}/${script}.d" || exit 0
+				hookdirs="${firsthookdir}/${script}.d"
+			fi
+		esac
+		run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" \$hookdirs
 		exit 0
 		EOF
 		chmod 755 "${pdir}/DEBIAN/${script}"
-- 
2.39.2

Re: [PATCH 1/1] scripts/package/builddeb: allow hooks also in /usr/share/kernel

[Masahiro Yamada]

On Thu, Nov 28, 2024 at 3:29 PM Johannes Schauer Marin Rodrigues
<josch@mister-muffin.de> wrote:
>
> By passing an additional directory to run-parts, allow Debian and its
> derivatives an easy way to ship maintainer scripts in /usr while at the
> same time allowing the local admin to easily override or disable them by
> placing hooks of the same name in /etc. This adds support for the
> mechanism described in the UAPI Configuration Files Specification for
> kernel hooks:
> https://uapi-group.org/specifications/specs/configuration_files_specification/
>
> This functionality relies on run-parts 5.21 or later.  It is the
> responsibility of packages installing hooks into /usr/share/kernel to
> also declare a Depends: debianutils (>= 5.21).
>
> KDEB_HOOKDIR can be used to change the list of directories that is
> searched. By default, /etc/kernel and /usr/share/kernel are hook
> directories.
>
> Signed-off-by: Johannes Schauer Marin Rodrigues <josch@mister-muffin.de>
> ---
>  scripts/package/builddeb | 44 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 36 insertions(+), 8 deletions(-)
>
> diff --git a/scripts/package/builddeb b/scripts/package/builddeb
> index 441b0bb66e0d..2772146a76ce 100755
> --- a/scripts/package/builddeb
> +++ b/scripts/package/builddeb
> @@ -5,10 +5,12 @@
>  #
>  # Simple script to generate a deb package for a Linux kernel. All the
>  # complexity of what to do with a kernel after it is installed or removed
> -# is left to other scripts and packages: they can install scripts in the
> -# /etc/kernel/{pre,post}{inst,rm}.d/ directories (or an alternative location
> -# specified in KDEB_HOOKDIR) that will be called on package install and
> -# removal.
> +# is left to other scripts and packages. Scripts can be placed into the
> +# preinst, postinst, prerm and postrm directories in /etc/kernel or
> +# /usr/share/kernel. A different list of search directories can be given
> +# via KDEB_HOOKDIR. Scripts in directories earlier in the list will
> +# override scripts of the same name in later directories.  The script will
> +# be called on package installation and removal.
>
>  set -eu
>
> @@ -68,11 +70,18 @@ install_linux_image () {
>         # kernel packages, as well as kernel packages built using make-kpkg.
>         # make-kpkg sets $INITRD to indicate whether an initramfs is wanted, and
>         # so do we; recent versions of dracut and initramfs-tools will obey this.
> -       debhookdir=${KDEB_HOOKDIR:-/etc/kernel}
> +       debhookdir=${KDEB_HOOKDIR:-/etc/kernel /usr/share/kernel}
> +
> +       # Only pre-create the first hook directory. Support for more than one hook
> +       # directory requires run-parts 5.21 and it is the responsibility of packages
> +       # creating additional hook directories to declare that dependency.
> +       firsthookdir=${debhookdir%% *}
>         for script in postinst postrm preinst prerm; do
> -               mkdir -p "${pdir}${debhookdir}/${script}.d"
> +               mkdir -p "${pdir}${firsthookdir}/${script}.d"
> +       done
>
> -               mkdir -p "${pdir}/DEBIAN"
> +       mkdir -p "${pdir}/DEBIAN"
> +       for script in postinst postrm preinst prerm; do
>                 cat <<-EOF > "${pdir}/DEBIAN/${script}"
>                 #!/bin/sh
>
> @@ -84,7 +93,26 @@ install_linux_image () {
>                 # Tell initramfs builder whether it's wanted
>                 export INITRD=$(if_enabled_echo CONFIG_BLK_DEV_INITRD Yes No)
>
> -               test -d ${debhookdir}/${script}.d && run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" ${debhookdir}/${script}.d
> +               # run-parts will error out if one of its directory arguments does not
> +               # exist, so filter the list of hook directories accordingly.
> +               hookdirs=
> +               for dir in ${debhookdir}; do
> +                       test -d "\$dir/${script}.d" || continue
> +                       hookdirs="\$hookdirs \$dir/${script}.d"
> +               done
> +               hookdirs="\${hookdirs# }"
> +               test -n "\$hookdirs" || exit 0
> +
> +               # If more than one hook directory remained, check version of run-parts. If
> +               # run-parts is too old, fall back to only processing the first.
> +               case \$hookdirs in *" "*) if ! run-parts --help 2>&1 \
> +                               | grep -Fxq "Usage: run-parts [OPTION]... DIRECTORY [DIRECTORY ...]"; then
> +                               echo "E: run-parts >=5.21 is required for multiple hook directories, falling back to $firsthookdir" >&2

Same comment as in the previous version.
If both /etc/kernel/postinst.d/ and /usr/share/kernel/postinst.d/ exist,
can we assume the run-parts>=5.12 on that system?

Do we need to check the help message and offer the fallback mechanism?






> +                               test -d "${firsthookdir}/${script}.d" || exit 0
> +                               hookdirs="${firsthookdir}/${script}.d"
> +                       fi
> +               esac
> +               run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" \$hookdirs
>                 exit 0
>                 EOF
>                 chmod 755 "${pdir}/DEBIAN/${script}"
> --
> 2.39.2
>
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 1/1] scripts/package/builddeb: allow hooks also in /usr/share/kernel

[Johannes Schauer Marin Rodrigues]

[-- Attachment #1: Type: text/plain, Size: 3848 bytes --]

Hi,

Quoting Masahiro Yamada (2024-12-02 16:42:02)
> > @@ -84,7 +93,26 @@ install_linux_image () { # Tell initramfs builder
> > whether it's wanted export INITRD=$(if_enabled_echo CONFIG_BLK_DEV_INITRD
> > Yes No)
> >
> > -               test -d ${debhookdir}/${script}.d && run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" ${debhookdir}/${script}.d
> > +               # run-parts will error out if one of its directory arguments does not
> > +               # exist, so filter the list of hook directories accordingly.
> > +               hookdirs=
> > +               for dir in ${debhookdir}; do
> > +                       test -d "\$dir/${script}.d" || continue
> > +                       hookdirs="\$hookdirs \$dir/${script}.d"
> > +               done
> > +               hookdirs="\${hookdirs# }"
> > +               test -n "\$hookdirs" || exit 0
> > +
> > +               # If more than one hook directory remained, check version of run-parts. If
> > +               # run-parts is too old, fall back to only processing the first.
> > +               case \$hookdirs in *" "*) if ! run-parts --help 2>&1 \
> > +                               | grep -Fxq "Usage: run-parts [OPTION]... DIRECTORY [DIRECTORY ...]"; then
> > +                               echo "E: run-parts >=5.21 is required for multiple hook directories, falling back to $firsthookdir" >&2
> 
> Same comment as in the previous version.
> If both /etc/kernel/postinst.d/ and /usr/share/kernel/postinst.d/ exist,
> can we assume the run-parts>=5.12 on that system?

since KDEB_HOOKDIR can now be any directories and any number of directories,
the question should rather be: if more than one directory from KDEB_HOOKDIR
exists, can we assume that run-parts>=5.12 exists on that system?

Personally, I'd prefer a best-effort fallback mechanism. The alternative would
be that kernel installation would just error out in case a (buggy) package on a
distro ships something in /usr/share/kernel/postinst.d/ but failed to also
declare a versioned dependency against debianutils. The error message cannot
(or rather only with considerable effort) tell the user *why* their kernel
installation errored out. By only considering the first hook directory
(probably /etc) in those situation, the kernel would succeed to install and the
hooks from the (buggy) package would be skipped. I understand that such a
behaviour comes with its own set of disadvantages. One could also argue, that
it is better to error out loudly in case of an error instead of hiding a
message prefixed with a "E:" in a bunch of console output when a kernel package
gets installed.

What is your position on this question? What behaviour would you prefer? If you
strongly prefer the kernel installation to error out loudly if run-parts is too
old, then my next patch will implement just that. I think whether "we can
assume run-parts>=5.12" depends on what we declare to be the right way to hold
this feature. If we say "packages must declare this versioned dependency and if
they fail to do this then it is their bug and not ours" then yes, then we can
assume run-parts>=5.12 in case of multiple directories.

> Do we need to check the help message and offer the fallback mechanism?

The answer two that question depends on the answer to the last question. If we
want to error out loudly with unsupported run-parts, then no help message has
to be checked. Otherwise, when we want to check what version of run-parts we
have, then there are two options. Either parsing the --version output (which is
not trivial itself because run-parts prints six lines of copyright information)
or parsing the --help output. The debianutils maintainer encouraged using the
latter option which is why I chose that one.

Thanks!

cheers, josch

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEElFhU6KL81LF4wVq58sulx4+9g+EFAmdN9VkACgkQ8sulx4+9
g+Hntw//RuQPuPkaWTE+A6z+6enRoy3qZb4RxvepPAt9FHm7HJtQ0IhyB0B3XOjG
e3EgQCzIm65fnrXDAGj7lJg/ablpnDENmJR98Eq7KzeIva5KQBemJzfoO1R+5eQO
9kh/es8YSR4KElviMMmmXBMRax/am5QZ+Ar+6ryqJYjduxvqxB2joMR7Ibqrq8vD
/jq8aZ7FkqwmqEZ52OicBr9HlOxwdBMOSWFNAxoUbmFFgcNl/7z0fO1T1gGQ+Kuq
xdEl+z15cZmPNqtUVIJl5Q/5TCbLmNpCG2iLH4u4vXNwu9/SsN0CZgOFkIOnZREJ
AFpIuoJbdGuUZOqk/06HAviliNLe3unJq+n3o1XIe1/Hz3QJLqyPgFfGxdW/ecQM
aXTvv7lAD70f1GldODCoGQEX5DAMhsnBH81/MJVMiQMJzkF52fBIwk4Hkk5NIzWh
EtHBGlFSzSMoW0RqHwZVD/JXi2zVvDDexfaHXaC2gtEPmG5o8qQcD8rUmzYx48u/
5884BKm0jqYjR55BMFq/Qx/G1jLo9TnDrYcjxPCLsdd9dF3Wh9oQmXk+z7ndCvrb
hD6c+69r2dtEbq33YR+xe9o+iqaOPcv813sLSFD+I/MTJtAFwGSMS2sbo1y2hgei
TbhaYjirBuSJfoCZK0Wj6KMZJPorSkddFXSk3oK975qrRBTJoE8=
=HThX
-----END PGP SIGNATURE-----

Re: [PATCH 1/1] scripts/package/builddeb: allow hooks also in /usr/share/kernel

[Masahiro Yamada]

On Tue, Dec 3, 2024 at 2:59 AM Johannes Schauer Marin Rodrigues
<josch@mister-muffin.de> wrote:
>
> Hi,
>
> Quoting Masahiro Yamada (2024-12-02 16:42:02)
> > > @@ -84,7 +93,26 @@ install_linux_image () { # Tell initramfs builder
> > > whether it's wanted export INITRD=$(if_enabled_echo CONFIG_BLK_DEV_INITRD
> > > Yes No)
> > >
> > > -               test -d ${debhookdir}/${script}.d && run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" ${debhookdir}/${script}.d
> > > +               # run-parts will error out if one of its directory arguments does not
> > > +               # exist, so filter the list of hook directories accordingly.
> > > +               hookdirs=
> > > +               for dir in ${debhookdir}; do
> > > +                       test -d "\$dir/${script}.d" || continue
> > > +                       hookdirs="\$hookdirs \$dir/${script}.d"
> > > +               done
> > > +               hookdirs="\${hookdirs# }"
> > > +               test -n "\$hookdirs" || exit 0
> > > +
> > > +               # If more than one hook directory remained, check version of run-parts. If
> > > +               # run-parts is too old, fall back to only processing the first.
> > > +               case \$hookdirs in *" "*) if ! run-parts --help 2>&1 \
> > > +                               | grep -Fxq "Usage: run-parts [OPTION]... DIRECTORY [DIRECTORY ...]"; then
> > > +                               echo "E: run-parts >=5.21 is required for multiple hook directories, falling back to $firsthookdir" >&2
> >
> > Same comment as in the previous version.
> > If both /etc/kernel/postinst.d/ and /usr/share/kernel/postinst.d/ exist,
> > can we assume the run-parts>=5.12 on that system?
>
> since KDEB_HOOKDIR can now be any directories and any number of directories,
> the question should rather be: if more than one directory from KDEB_HOOKDIR
> exists, can we assume that run-parts>=5.12 exists on that system?
>
> Personally, I'd prefer a best-effort fallback mechanism. The alternative would
> be that kernel installation would just error out in case a (buggy) package on a
> distro ships something in /usr/share/kernel/postinst.d/ but failed to also
> declare a versioned dependency against debianutils. The error message cannot
> (or rather only with considerable effort) tell the user *why* their kernel
> installation errored out. By only considering the first hook directory
> (probably /etc) in those situation, the kernel would succeed to install and the
> hooks from the (buggy) package would be skipped. I understand that such a
> behaviour comes with its own set of disadvantages. One could also argue, that
> it is better to error out loudly in case of an error instead of hiding a
> message prefixed with a "E:" in a bunch of console output when a kernel package
> gets installed.
>
> What is your position on this question? What behaviour would you prefer? If you
> strongly prefer the kernel installation to error out loudly if run-parts is too
> old, then my next patch will implement just that. I think whether "we can
> assume run-parts>=5.12" depends on what we declare to be the right way to hold
> this feature. If we say "packages must declare this versioned dependency and if
> they fail to do this then it is their bug and not ours" then yes, then we can
> assume run-parts>=5.12 in case of multiple directories.

My preference is to pass the existing hook directories to run-parts.
If KDEB_HOOKDIR specifies two directories and both exist,
pass them to run-parts.



>
> > Do we need to check the help message and offer the fallback mechanism?
>
> The answer two that question depends on the answer to the last question. If we
> want to error out loudly with unsupported run-parts, then no help message has
> to be checked. Otherwise, when we want to check what version of run-parts we
> have, then there are two options. Either parsing the --version output (which is
> not trivial itself because run-parts prints six lines of copyright information)
> or parsing the --help output. The debianutils maintainer encouraged using the
> latter option which is why I chose that one.
>
> Thanks!
>
> cheers, josch



-- 
Best Regards
Masahiro Yamada

[PATCHv3 0/1] scripts/package/builddeb: allow hooks also in /usr/share/kernel

[Johannes Schauer Marin Rodrigues]

Hi,

Quoting Masahiro Yamada (2024-12-03 07:15:17)
> On Tue, Dec 3, 2024 at 2:59 AM Johannes Schauer Marin Rodrigues
> <josch@mister-muffin.de> wrote:
> >
> > Hi,
> >
> > Quoting Masahiro Yamada (2024-12-02 16:42:02)
> > > > @@ -84,7 +93,26 @@ install_linux_image () { # Tell initramfs builder
> > > > whether it's wanted export INITRD=$(if_enabled_echo CONFIG_BLK_DEV_INITRD
> > > > Yes No)
> > > >
> > > > -               test -d ${debhookdir}/${script}.d && run-parts --arg="${KERNELRELEASE}" --  arg="/${installed_image_path}" ${debhookdir}/${script}.d
> > > > +               # run-parts will error out if one of its directory arguments does not
> > > > +               # exist, so filter the list of hook directories accordingly.
> > > > +               hookdirs=
> > > > +               for dir in ${debhookdir}; do
> > > > +                       test -d "\$dir/${script}.d" || continue
> > > > +                       hookdirs="\$hookdirs \$dir/${script}.d"
> > > > +               done
> > > > +               hookdirs="\${hookdirs# }"
> > > > +               test -n "\$hookdirs" || exit 0
> > > > +
> > > > +               # If more than one hook directory remained, check version of run-parts. If
> > > > +               # run-parts is too old, fall back to only processing the first.
> > > > +               case \$hookdirs in *" "*) if ! run-parts --help 2>&1 \
> > > > +                               | grep -Fxq "Usage: run-parts [OPTION]... DIRECTORY         [DIRECTORY ...]"; then
> > > > +                               echo "E: run-parts >=5.21 is required for multiple hook     directories, falling back to $firsthookdir" >&2
> > >
> > > Same comment as in the previous version.
> > > If both /etc/kernel/postinst.d/ and /usr/share/kernel/postinst.d/ exist,
> > > can we assume the run-parts>=5.12 on that system?
> >
> >
> > since KDEB_HOOKDIR can now be any directories and any number of directories,
> > the question should rather be: if more than one directory from KDEB_HOOKDIR 
> > exists, can we assume that run-parts>=5.12 exists on that system?
> >
> > Personally, I'd prefer a best-effort fallback mechanism. The alternative would
> > be that kernel installation would just error out in case a (buggy) package on a
> > distro ships something in /usr/share/kernel/postinst.d/ but failed to also
> > declare a versioned dependency against debianutils. The error message cannot
> > (or rather only with considerable effort) tell the user *why* their kernel
> > installation errored out. By only considering the first hook directory
> > (probably /etc) in those situation, the kernel would succeed to install and the
> > hooks from the (buggy) package would be skipped. I understand that such a
> > behaviour comes with its own set of disadvantages. One could also argue, that
> > it is better to error out loudly in case of an error instead of hiding a
> > message prefixed with a "E:" in a bunch of console output when a kernel package
> > gets installed.
> >
> > What is your position on this question? What behaviour would you prefer? If you
> > strongly prefer the kernel installation to error out loudly if run-parts is too
> > old, then my next patch will implement just that. I think whether "we can
> > assume run-parts>=5.12" depends on what we declare to be the right way to hold
> > this feature. If we say "packages must declare this versioned dependency and if
> > they fail to do this then it is their bug and not ours" then yes, then we can
> > assume run-parts>=5.12 in case of multiple directories.
>█
> My preference is to pass the existing hook directories to run-parts.
> If KDEB_HOOKDIR specifies two directories and both exist,
> pass them to run-parts.

Done in this third version of my patch. Thank you for your reviews and your
patience with me! :)

cheers, josch

Johannes Schauer Marin Rodrigues (1):
  scripts/package/builddeb: allow hooks also in /usr/share/kernel

 scripts/package/builddeb | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

-- 
2.39.2

[PATCH 1/1] scripts/package/builddeb: allow hooks also in /usr/share/kernel

[Johannes Schauer Marin Rodrigues]

By passing an additional directory to run-parts, allow Debian and its
derivatives to ship maintainer scripts in /usr while at the same time
allowing the local admin to override or disable them by placing hooks of
the same name in /etc. This adds support for the mechanism described in
the UAPI Configuration Files Specification for kernel hooks. The same
idea is also used by udev, systemd or modprobe for their config files.
https://uapi-group.org/specifications/specs/configuration_files_specification/

This functionality relies on run-parts 5.21 or later.  It is the
responsibility of packages installing hooks into /usr/share/kernel to
also declare a Depends: debianutils (>= 5.21).

KDEB_HOOKDIR can be used to change the list of directories that is
searched. By default, /etc/kernel and /usr/share/kernel are hook
directories.

Signed-off-by: Johannes Schauer Marin Rodrigues <josch@mister-muffin.de>
---
 scripts/package/builddeb | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/scripts/package/builddeb b/scripts/package/builddeb
index 441b0bb66e0d..6e83f6f3ec6d 100755
--- a/scripts/package/builddeb
+++ b/scripts/package/builddeb
@@ -5,10 +5,12 @@
 #
 # Simple script to generate a deb package for a Linux kernel. All the
 # complexity of what to do with a kernel after it is installed or removed
-# is left to other scripts and packages: they can install scripts in the
-# /etc/kernel/{pre,post}{inst,rm}.d/ directories (or an alternative location
-# specified in KDEB_HOOKDIR) that will be called on package install and
-# removal.
+# is left to other scripts and packages. Scripts can be placed into the
+# preinst, postinst, prerm and postrm directories in /etc/kernel or
+# /usr/share/kernel. A different list of search directories can be given
+# via KDEB_HOOKDIR. Scripts in directories earlier in the list will
+# override scripts of the same name in later directories.  The script will
+# be called on package installation and removal.
 
 set -eu
 
@@ -68,11 +70,18 @@ install_linux_image () {
 	# kernel packages, as well as kernel packages built using make-kpkg.
 	# make-kpkg sets $INITRD to indicate whether an initramfs is wanted, and
 	# so do we; recent versions of dracut and initramfs-tools will obey this.
-	debhookdir=${KDEB_HOOKDIR:-/etc/kernel}
+	debhookdir=${KDEB_HOOKDIR:-/etc/kernel /usr/share/kernel}
+
+	# Only pre-create the first hook directory. Support for more than one hook
+	# directory requires run-parts 5.21 and it is the responsibility of packages
+	# creating additional hook directories to declare that dependency.
+	firsthookdir=${debhookdir%% *}
 	for script in postinst postrm preinst prerm; do
-		mkdir -p "${pdir}${debhookdir}/${script}.d"
+		mkdir -p "${pdir}${firsthookdir}/${script}.d"
+	done
 
-		mkdir -p "${pdir}/DEBIAN"
+	mkdir -p "${pdir}/DEBIAN"
+	for script in postinst postrm preinst prerm; do
 		cat <<-EOF > "${pdir}/DEBIAN/${script}"
 		#!/bin/sh
 
@@ -84,7 +93,15 @@ install_linux_image () {
 		# Tell initramfs builder whether it's wanted
 		export INITRD=$(if_enabled_echo CONFIG_BLK_DEV_INITRD Yes No)
 
-		test -d ${debhookdir}/${script}.d && run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" ${debhookdir}/${script}.d
+		# run-parts will error out if one of its directory arguments does not
+		# exist, so filter the list of hook directories accordingly.
+		hookdirs=
+		for dir in ${debhookdir}; do
+			test -d "\$dir/${script}.d" || continue
+			hookdirs="\$hookdirs \$dir/${script}.d"
+		done
+		hookdirs="\${hookdirs# }"
+		test -n "\$hookdirs" && run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" \$hookdirs
 		exit 0
 		EOF
 		chmod 755 "${pdir}/DEBIAN/${script}"
-- 
2.39.2

Re: [PATCH 1/1] scripts/package/builddeb: allow hooks also in /usr/share/kernel

[Masahiro Yamada]

On Tue, Dec 3, 2024 at 3:55 PM Johannes Schauer Marin Rodrigues
<josch@mister-muffin.de> wrote:
>
> By passing an additional directory to run-parts, allow Debian and its
> derivatives to ship maintainer scripts in /usr while at the same time
> allowing the local admin to override or disable them by placing hooks of
> the same name in /etc. This adds support for the mechanism described in
> the UAPI Configuration Files Specification for kernel hooks. The same
> idea is also used by udev, systemd or modprobe for their config files.
> https://uapi-group.org/specifications/specs/configuration_files_specification/
>
> This functionality relies on run-parts 5.21 or later.  It is the
> responsibility of packages installing hooks into /usr/share/kernel to
> also declare a Depends: debianutils (>= 5.21).
>
> KDEB_HOOKDIR can be used to change the list of directories that is
> searched. By default, /etc/kernel and /usr/share/kernel are hook
> directories.
>
> Signed-off-by: Johannes Schauer Marin Rodrigues <josch@mister-muffin.de>
> ---
>  scripts/package/builddeb | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/scripts/package/builddeb b/scripts/package/builddeb
> index 441b0bb66e0d..6e83f6f3ec6d 100755
> --- a/scripts/package/builddeb
> +++ b/scripts/package/builddeb
> @@ -5,10 +5,12 @@
>  #
>  # Simple script to generate a deb package for a Linux kernel. All the
>  # complexity of what to do with a kernel after it is installed or removed
> -# is left to other scripts and packages: they can install scripts in the
> -# /etc/kernel/{pre,post}{inst,rm}.d/ directories (or an alternative location
> -# specified in KDEB_HOOKDIR) that will be called on package install and
> -# removal.
> +# is left to other scripts and packages. Scripts can be placed into the
> +# preinst, postinst, prerm and postrm directories in /etc/kernel or
> +# /usr/share/kernel. A different list of search directories can be given
> +# via KDEB_HOOKDIR. Scripts in directories earlier in the list will
> +# override scripts of the same name in later directories.  The script will
> +# be called on package installation and removal.
>
>  set -eu
>
> @@ -68,11 +70,18 @@ install_linux_image () {
>         # kernel packages, as well as kernel packages built using make-kpkg.
>         # make-kpkg sets $INITRD to indicate whether an initramfs is wanted, and
>         # so do we; recent versions of dracut and initramfs-tools will obey this.
> -       debhookdir=${KDEB_HOOKDIR:-/etc/kernel}
> +       debhookdir=${KDEB_HOOKDIR:-/etc/kernel /usr/share/kernel}
> +
> +       # Only pre-create the first hook directory. Support for more than one hook
> +       # directory requires run-parts 5.21 and it is the responsibility of packages
> +       # creating additional hook directories to declare that dependency.
> +       firsthookdir=${debhookdir%% *}
>         for script in postinst postrm preinst prerm; do
> -               mkdir -p "${pdir}${debhookdir}/${script}.d"
> +               mkdir -p "${pdir}${firsthookdir}/${script}.d"


I still do not understand why this 'mkdir' is needed.

linux-image package does not install any script into the hook directory.
In general, there exist some scripts (e.g. initramfs-tools) already
under /etc/kernel/*.d/  (and under /usr/share/kernel/*.d/ once the
new location is used broader).
If nothing exists under the hook directory, there is no point to
execute run-parts.




> +       done
>
> -               mkdir -p "${pdir}/DEBIAN"
> +       mkdir -p "${pdir}/DEBIAN"

Please drop this noise change.

If you want to optimize this, please split it into a separate patch like
"kbuild: deb-pkg: create DEBIAN directory just once" etc.




> +       for script in postinst postrm preinst prerm; do
>                 cat <<-EOF > "${pdir}/DEBIAN/${script}"
>                 #!/bin/sh
>
> @@ -84,7 +93,15 @@ install_linux_image () {
>                 # Tell initramfs builder whether it's wanted
>                 export INITRD=$(if_enabled_echo CONFIG_BLK_DEV_INITRD Yes No)
>
> -               test -d ${debhookdir}/${script}.d && run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" ${debhookdir}/${script}.d
> +               # run-parts will error out if one of its directory arguments does not
> +               # exist, so filter the list of hook directories accordingly.
> +               hookdirs=
> +               for dir in ${debhookdir}; do
> +                       test -d "\$dir/${script}.d" || continue
> +                       hookdirs="\$hookdirs \$dir/${script}.d"
> +               done
> +               hookdirs="\${hookdirs# }"
> +               test -n "\$hookdirs" && run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" \$hookdirs
>                 exit 0
>                 EOF
>                 chmod 755 "${pdir}/DEBIAN/${script}"
> --
> 2.39.2
>
>
--
Best Regards
Masahiro Yamada

Re: [PATCH 1/1] scripts/package/builddeb: allow hooks also in /usr/share/kernel

[Johannes Schauer Marin Rodrigues]

[-- Attachment #1: Type: text/plain, Size: 2105 bytes --]

Quoting Masahiro Yamada (2024-12-03 10:27:11)
> > @@ -68,11 +70,18 @@ install_linux_image () {
> >         # kernel packages, as well as kernel packages built using make-kpkg.
> >         # make-kpkg sets $INITRD to indicate whether an initramfs is wanted, and
> >         # so do we; recent versions of dracut and initramfs-tools will obey this.
> > -       debhookdir=${KDEB_HOOKDIR:-/etc/kernel}
> > +       debhookdir=${KDEB_HOOKDIR:-/etc/kernel /usr/share/kernel}
> > +
> > +       # Only pre-create the first hook directory. Support for more than one hook
> > +       # directory requires run-parts 5.21 and it is the responsibility of packages
> > +       # creating additional hook directories to declare that dependency.
> > +       firsthookdir=${debhookdir%% *}
> >         for script in postinst postrm preinst prerm; do
> > -               mkdir -p "${pdir}${debhookdir}/${script}.d"
> > +               mkdir -p "${pdir}${firsthookdir}/${script}.d"
> 
> I still do not understand why this 'mkdir' is needed.
> 
> linux-image package does not install any script into the hook directory.
> In general, there exist some scripts (e.g. initramfs-tools) already
> under /etc/kernel/*.d/  (and under /usr/share/kernel/*.d/ once the
> new location is used broader).
> If nothing exists under the hook directory, there is no point to
> execute run-parts.

Unless I'm misunderstanding the old code, the existing script *does* create
/etc/kernel/*.d/ (That's the "- mkdir -p" line above) so I wanted to preserve
this behaviour even with more than one directory in KDEB_HOOKDIR. Do I
misunderstand something? Are you saying that with this change, no
/etc/kernel/*.d/ should be created anymore? Why?

> > +       done
> >
> > -               mkdir -p "${pdir}/DEBIAN"
> > +       mkdir -p "${pdir}/DEBIAN"
> 
> Please drop this noise change.
> 
> If you want to optimize this, please split it into a separate patch like
> "kbuild: deb-pkg: create DEBIAN directory just once" etc.

Okay, no need to optimize this now. mkdir -p is cheap.

Thanks!

cheers, josch

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEElFhU6KL81LF4wVq58sulx4+9g+EFAmdO0iQACgkQ8sulx4+9
g+G84w/8C12soywhuafBv5UkSJTU0Wwbpjn3vo9lsGRlZFhWTG33gAtNE8EDKDJH
SMSrC8+BeopYAKXDw80aQdupSbgYE2Ksi2os2uTQT+BtrMenbTvT5HWvUyLYPeqa
OKxgxGDMDoEch/SgxYJAlqt2R6D7bBb7kkeub0y6xwmPMZ+oiiAJPYOUagvSxFlK
d9DtHf/o75hGv5oQpB9QaGQP51XavyvXeFCmf0R86Alh2p5CcMFnfpdb7q6YSGcf
6IvpFm7gSJCyYp7CMz5DirxXfI3P1Bk4qOaWZ938jyk4Ts1TRJbJNLIk25jjDoLA
t4/WTAySBi+ELMu9BF0WWcjjXKK3gMXizR/X9ejqEjOKVylXEWaSg0PZgWF/Neey
z8eCumAvtIav2NOUeGqsq5/uo9j0swIvERqnJBrhF7yHQAYmrBkRb3OUHxU7pTL5
5Nu6xdXRty5txgeqFMwJomG4KcvAcc2/V3sK8eggXYXu9Hty9cxhsy4gTexxws5h
Ss3wbx3Uvtt1ADFoFQ2WVI9u/2fgkQSP9PgsBEyifY3Qfykn3xBF3aCYbLuAA5k5
avteIg+JNZbhLiWa9RkEgcQVkBJ01WEE+KhtPYNjG+OKEejwmRXM5nIhPupcOzZ9
jYL6QueXHbDwvxXkf0YV6Rw2ZqmSa5IiB4biZGHoqH8srR6iHAI=
=yu9k
-----END PGP SIGNATURE-----

Re: [PATCH 1/1] scripts/package/builddeb: allow hooks also in /usr/share/kernel

[Masahiro Yamada]

On Tue, Dec 3, 2024 at 6:41 PM Johannes Schauer Marin Rodrigues
<josch@mister-muffin.de> wrote:
>
> Quoting Masahiro Yamada (2024-12-03 10:27:11)
> > > @@ -68,11 +70,18 @@ install_linux_image () {
> > >         # kernel packages, as well as kernel packages built using make-kpkg.
> > >         # make-kpkg sets $INITRD to indicate whether an initramfs is wanted, and
> > >         # so do we; recent versions of dracut and initramfs-tools will obey this.
> > > -       debhookdir=${KDEB_HOOKDIR:-/etc/kernel}
> > > +       debhookdir=${KDEB_HOOKDIR:-/etc/kernel /usr/share/kernel}
> > > +
> > > +       # Only pre-create the first hook directory. Support for more than one hook
> > > +       # directory requires run-parts 5.21 and it is the responsibility of packages
> > > +       # creating additional hook directories to declare that dependency.
> > > +       firsthookdir=${debhookdir%% *}
> > >         for script in postinst postrm preinst prerm; do
> > > -               mkdir -p "${pdir}${debhookdir}/${script}.d"
> > > +               mkdir -p "${pdir}${firsthookdir}/${script}.d"
> >
> > I still do not understand why this 'mkdir' is needed.
> >
> > linux-image package does not install any script into the hook directory.
> > In general, there exist some scripts (e.g. initramfs-tools) already
> > under /etc/kernel/*.d/  (and under /usr/share/kernel/*.d/ once the
> > new location is used broader).
> > If nothing exists under the hook directory, there is no point to
> > execute run-parts.
>
> Unless I'm misunderstanding the old code, the existing script *does* create
> /etc/kernel/*.d/ (That's the "- mkdir -p" line above) so I wanted to preserve
> this behaviour even with more than one directory in KDEB_HOOKDIR. Do I
> misunderstand something?

Right. The existing code does create empty directories, which
are unnecessary.


> Are you saying that with this change, no
> /etc/kernel/*.d/ should be created anymore? Why?


The 'mkdir' is unnecessary regardless of your patch,
unless I am misunderstanding the code.

At present, it is a single line of code.
You are extending it into several lines of verbose code simply
in order to precisely retain these unnecessary directories.
This is a problem for me because I will need to maintain code
whose necessity is unclear.

Judging from your cautious approach and verbose changes, I
assume you are trying to avoid any risks (a common trait
among many contributors).

That said, I understand you are not motivated to strive for
clean code at all costs. Once you commit the run-parts
changes, you may feel your work is done. However, as the
maintainer, I must manage this code for many years,
so I aim to proactively remove unneeded code.

I have decided to take responsibility for cleaning up this
single line myself:

https://lore.kernel.org/linux-kbuild/CAK7LNARU=M282fAOOgzPOBPtDNFPjH8To9eK2vYstWxkEDEEPA@mail.gmail.com/T/#t

If something breaks due to missing directories,
it will be my fault, not yours.

Now that the dummy directories are gone from the linux-image
package, please prepare the next version without the
"pre-create the first hook directory" stuff.

A few more requests.

Please add the version number (the next patch will be v4?) like others do.
And "kbuild: deb-pkg:" as the patch subject.
('git log script/package/buildeb' to see examples)



>
> > > +       done
> > >
> > > -               mkdir -p "${pdir}/DEBIAN"
> > > +       mkdir -p "${pdir}/DEBIAN"
> >
> > Please drop this noise change.
> >
> > If you want to optimize this, please split it into a separate patch like
> > "kbuild: deb-pkg: create DEBIAN directory just once" etc.
>
> Okay, no need to optimize this now. mkdir -p is cheap.
>
> Thanks!
>
> cheers, josch
--
Best Regards
Masahiro Yamada

[PATCHv4 0/1] kbuild: deb-pkg: allow hooks also in /usr/share/kernel

[Johannes Schauer Marin Rodrigues]

Quoting Masahiro Yamada (2024-12-03 15:24:10)
> > Unless I'm misunderstanding the old code, the existing script *does* create 
> > /etc/kernel/*.d/ (That's the "- mkdir -p" line above) so I wanted to
> > preserve this behaviour even with more than one directory in KDEB_HOOKDIR.
> > Do I misunderstand something?
>█
> Right. The existing code does create empty directories, which
> are unnecessary.

Okay. I had assumed that their creation was intentional.

> > Are you saying that with this change, no
> > /etc/kernel/*.d/ should be created anymore? Why?
>█
> The 'mkdir' is unnecessary regardless of your patch,
> unless I am misunderstanding the code.

Yes, the mkdir is not necessary as far as I can see. Packages that do
install hooks will take care of creating these. It doesn't have to be
the linux kernel image package that takes care of creating them.

> Judging from your cautious approach and verbose changes, I
> assume you are trying to avoid any risks (a common trait
> among many contributors).

That is correct. I hope that such an approach is considered desirable?

> That said, I understand you are not motivated to strive for
> clean code at all costs. Once you commit the run-parts
> changes, you may feel your work is done. However, as the
> maintainer, I must manage this code for many years,
> so I aim to proactively remove unneeded code.

I understand. I don't see a problem changing the patch in a way such that you
feel that you can take responsibility for it in the long term. Thank you for
taking the time to explain this to me. It is really much appreciated and I know
that you don't have to. Thank you!

That being said, please do reach out to me if there are problems with my
contributions later on. I am also the contributor of the new functionality in
run-parts, so should there be issues with debianutils, you can also contact me
about problems with using run-parts on multiple directories as well.

> I have decided to take responsibility for cleaning up this
> single line myself:
>█
> https://lore.kernel.org/linux-kbuild/CAK7LNARU=M282fAOOgzPOBPtDNFPjH8To9eK2vYstWxkEDEEPA@mail.gmail.com/T/#t
>█
> If something breaks due to missing directories,
> it will be my fault, not yours.

Thank you. My new patch (v4) in the next mail now is on top of your
change above.

> Now that the dummy directories are gone from the linux-image
> package, please prepare the next version without the
> "pre-create the first hook directory" stuff.
>█
> A few more requests.
>█
> Please add the version number (the next patch will be v4?) like others do.
> And "kbuild: deb-pkg:" as the patch subject.  ('git log
> script/package/buildeb' to see examples)

I think I have taken care of these issues now.

Thank you!

cheers, josch


Johannes Schauer Marin Rodrigues (1):
  kbuild: deb-pkg: allow hooks also in /usr/share/kernel

 scripts/package/builddeb | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

-- 
2.39.2

[PATCHv4 1/1] kbuild: deb-pkg: allow hooks also in /usr/share/kernel

[Johannes Schauer Marin Rodrigues]

By passing an additional directory to run-parts, allow Debian and its
derivatives to ship maintainer scripts in /usr while at the same time
allowing the local admin to override or disable them by placing hooks of
the same name in /etc. This adds support for the mechanism described in
the UAPI Configuration Files Specification for kernel hooks. The same
idea is also used by udev, systemd or modprobe for their config files.
https://uapi-group.org/specifications/specs/configuration_files_specification/

This functionality relies on run-parts 5.21 or later.  It is the
responsibility of packages installing hooks into /usr/share/kernel to
also declare a Depends: debianutils (>= 5.21).

KDEB_HOOKDIR can be used to change the list of directories that is
searched. By default, /etc/kernel and /usr/share/kernel are hook
directories. Since the list of directories in KDEB_HOOKDIR is separated
by spaces, the paths must not contain the space character themselves.

Signed-off-by: Johannes Schauer Marin Rodrigues <josch@mister-muffin.de>
---
 scripts/package/builddeb | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/scripts/package/builddeb b/scripts/package/builddeb
index 5843c5c04e56..1b79f25442c7 100755
--- a/scripts/package/builddeb
+++ b/scripts/package/builddeb
@@ -5,10 +5,12 @@
 #
 # Simple script to generate a deb package for a Linux kernel. All the
 # complexity of what to do with a kernel after it is installed or removed
-# is left to other scripts and packages: they can install scripts in the
-# /etc/kernel/{pre,post}{inst,rm}.d/ directories (or an alternative location
-# specified in KDEB_HOOKDIR) that will be called on package install and
-# removal.
+# is left to other scripts and packages. Scripts can be placed into the
+# preinst, postinst, prerm and postrm directories in /etc/kernel or
+# /usr/share/kernel. A different list of search directories can be given
+# via KDEB_HOOKDIR. Scripts in directories earlier in the list will
+# override scripts of the same name in later directories.  The script will
+# be called on package installation and removal.
 
 set -eu
 
@@ -68,7 +70,8 @@ install_linux_image () {
 	# kernel packages, as well as kernel packages built using make-kpkg.
 	# make-kpkg sets $INITRD to indicate whether an initramfs is wanted, and
 	# so do we; recent versions of dracut and initramfs-tools will obey this.
-	debhookdir=${KDEB_HOOKDIR:-/etc/kernel}
+	debhookdir=${KDEB_HOOKDIR:-/etc/kernel /usr/share/kernel}
+
 	for script in postinst postrm preinst prerm; do
 		mkdir -p "${pdir}/DEBIAN"
 		cat <<-EOF > "${pdir}/DEBIAN/${script}"
@@ -82,7 +85,15 @@ install_linux_image () {
 		# Tell initramfs builder whether it's wanted
 		export INITRD=$(if_enabled_echo CONFIG_BLK_DEV_INITRD Yes No)
 
-		test -d ${debhookdir}/${script}.d && run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" ${debhookdir}/${script}.d
+		# run-parts will error out if one of its directory arguments does not
+		# exist, so filter the list of hook directories accordingly.
+		hookdirs=
+		for dir in ${debhookdir}; do
+			test -d "\$dir/${script}.d" || continue
+			hookdirs="\$hookdirs \$dir/${script}.d"
+		done
+		hookdirs="\${hookdirs# }"
+		test -n "\$hookdirs" && run-parts --arg="${KERNELRELEASE}" --arg="/${installed_image_path}" \$hookdirs
 		exit 0
 		EOF
 		chmod 755 "${pdir}/DEBIAN/${script}"
-- 
2.39.2

Re: [PATCHv4 1/1] kbuild: deb-pkg: allow hooks also in /usr/share/kernel

[Masahiro Yamada]

On Wed, Dec 4, 2024 at 1:17 AM Johannes Schauer Marin Rodrigues
<josch@mister-muffin.de> wrote:
>
> By passing an additional directory to run-parts, allow Debian and its
> derivatives to ship maintainer scripts in /usr while at the same time
> allowing the local admin to override or disable them by placing hooks of
> the same name in /etc. This adds support for the mechanism described in
> the UAPI Configuration Files Specification for kernel hooks. The same
> idea is also used by udev, systemd or modprobe for their config files.
> https://uapi-group.org/specifications/specs/configuration_files_specification/
>
> This functionality relies on run-parts 5.21 or later.  It is the
> responsibility of packages installing hooks into /usr/share/kernel to
> also declare a Depends: debianutils (>= 5.21).
>
> KDEB_HOOKDIR can be used to change the list of directories that is
> searched. By default, /etc/kernel and /usr/share/kernel are hook
> directories. Since the list of directories in KDEB_HOOKDIR is separated
> by spaces, the paths must not contain the space character themselves.
>
> Signed-off-by: Johannes Schauer Marin Rodrigues <josch@mister-muffin.de>
> ---

Applied to linux-kbuild. Thanks!




-- 
Best Regards
Masahiro Yamada