Re: [PATCH] alsa-utils: Install and delete from the same udev-rules-dir

[Tanu Kaskinen <tanuk@iki.fi>]

On Thu, 2017-09-21 at 16:56 +0200, Ola x Nilsson wrote:
> The --with-udev-rules-dir option used with udev is exactly what the
> configure script uses, so there is no need for it.
> On the other hand, if we do not have an udev.pc file we should tell
> alsa-utils where to install the rules so we know where they should be
> deleted from.
> 
> Signed-off-by: Ola x Nilsson <olani@axis.com>
> ---
>  meta/recipes-multimedia/alsa/alsa-utils_1.1.4.bb | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/meta/recipes-multimedia/alsa/alsa-utils_1.1.4.bb b/meta/recipes-multimedia/alsa/alsa-utils_1.1.4.bb
> index c8f4b861bd..7fc6c29a61 100644
> --- a/meta/recipes-multimedia/alsa/alsa-utils_1.1.4.bb
> +++ b/meta/recipes-multimedia/alsa/alsa-utils_1.1.4.bb
> @@ -9,14 +9,17 @@ DEPENDS = "alsa-lib ncurses libsamplerate0"
>  
>  PACKAGECONFIG ??= "udev"
>  
> +udevdir = "${nonarch_base_libdir}/udev"
> +
>  # alsabat can be built also without fftw support (with reduced functionality).
>  # It would be better to always enable alsabat, but provide an option for
>  # enabling/disabling fftw. The configure script doesn't support that, however
>  # (at least in any obvious way), so for now we only support alsabat with fftw
>  # or no alsabat at all.
>  PACKAGECONFIG[bat] = "--enable-bat,--disable-bat,fftwf"
> -
> -PACKAGECONFIG[udev] = "--with-udev-rules-dir=`pkg-config --variable=udevdir udev`/rules.d,,udev"
> +# The udev-rules dir is taken from udev.pc.  Set the dir explicitly
> +# when udev is not in our DEPENDS.
> +PACKAGECONFIG[udev] = ",--with-udev-rules-dir=${udevdir}/rules.d,udev"

I'll be reading this recipe every once in a while, and I feel this
comment won't be easy to understand after some time. My suggestion for
an alternative:

# If udev is not in PACKAGECONFIG, we will leave the udev rules
# unpackaged and remove them in do_install(). When removing the rules,
# we need to know where they were installed, which is why we set the
# udev-rules-dir variable here.

>  PACKAGECONFIG[manpages] = "--enable-xmlto, --disable-xmlto, xmlto-native docbook-xml-dtd4-native docbook-xsl-stylesheets-native"
>  
>  SRC_URI = "ftp://ftp.alsa-project.org/pub/utils/alsa-utils-${PV}.tar.bz2 \
> @@ -101,9 +104,13 @@ do_install() {
>  	rm ${D}${sbindir}/alsa-info.sh
>  	rm -f ${D}${sbindir}/alsabat-test.sh
>  
> -	if ${@bb.utils.contains('PACKAGECONFIG', 'udev', 'false', 'true', d)}; then
> -		# This is where alsa-utils will install its rules if we don't tell it anything else.
> -		rm -rf ${D}${nonarch_base_libdir}/udev
> -		rmdir --ignore-fail-on-non-empty ${D}${nonarch_base_libdir}
> +	if [ "${@bb.utils.filter('PACKAGECONFIG', 'udev', d)}" ]; then
> +		# This is where alsa-utils installs if there is an udev.pc file
> +		udevdir=$(pkg-config --variable=udevdir udev)
> +	else
> +		# This is where we told alsa-utils to install the rules
> +		udevdir=${udevdir}
>  	fi
> +	rm -rf ${D}$udevdir
> +	rmdir --ignore-fail-on-non-empty ${D}${udevdir%/*}

This will remove the udev rules also when udev is enabled. Previously
they were removed only when udev was disabled. I suppose this is a
mistake, since you didn't indicate this kind of change in the commit
message.

Some comment about what you're doing would be nice when using
${udevdir%/*}. At least I didn't understand what is happening here
without reading the bash manual.

-- 
Tanu

https://www.patreon.com/tanuk