Subject: Re: [PATCH gnome-team v7 1/5] gnu: eudev: Update libudev version to 251.

From: Maxim Cournoyer <[email-masked]>
Date: Thu, 05 Oct 2023 09:28:50 -0400

Hello,

Vivien Kraus <[email-masked]> writes:

> Eudev has significant improvements from 3.2.12, but they are not released
> yet. The package version number has already been bumped to 3.2.14, but since
> it is not released yet, we still advertise it as 3.2.12.
>
> Everything that eudev searches in "sysconf" (/etc/udev/*) is actually searched
> under its installation prefix. The udev-service-type however prepares every
> file in /etc/udev, without a prefix.
>
> Eudev has a hardware database that installs descriptions of hardware in /etc,
> but they should go to <prefix>/lib prior to being used. The build system can’t
> install to /etc, and should not, because this directory is owned by the
> udev-service-type. So they are installed directly in <prefix>/lib. Another
> file, an empty /etc/udev.conf, is simply ignored.

This sounds more like a limitation/bug in our udev-service-type than in
eudev?  If eudev wants its files installed to /etc, they should be left
there I think.  Going against this is more maintenance down the road,
and surprise from users.

> The hwdb.bin file used to be generated in <prefix>/etc/udev/hwdb.bin, but
> since the sysconf dir is now directly /etc, the hwdb.bin index will not be
> found under <prefix>/etc/udev/hwdb.bin.
>
> * gnu/packages/linux.scm (eudev): Update to a post-v3.2.12 commit.
> [snippet]: New snippet to override the version number.
> [modules]: Import (guix build utils).
> [#:phases] <allow-eudev-hwdb>: New phase.
> <install-in-lib>: New phase.
> <build-hwdb>: Remove phase.
> [#:configure-flags]: Set sysconfdir to avoid a prefix.

The above should be commented in the code.  What prefix?  sysconfdir
typically defaults to /etc, if I recall correctly.

> [native-search-paths]: Add UDEV_HWDB_PATH.
> * gnu/packages/patches/eudev-rules-directory.patch: Rebase it.
> ---
>  gnu/packages/linux.scm                        | 64 ++++++++++++++-----
>  .../patches/eudev-rules-directory.patch       |  9 +--
>  2 files changed, 52 insertions(+), 21 deletions(-)
>
> diff --git a/gnu/packages/linux.scm b/gnu/packages/linux.scm
> index 85e3d9845d..e75ed0b233 100644
> --- a/gnu/packages/linux.scm
> +++ b/gnu/packages/linux.scm
> @@ -4263,18 +4263,31 @@ (define-public earlyoom
>  
>  (define-public eudev
>    ;; The post-systemd fork, maintained by Gentoo.
> +  (define commit
> +    "c5bae0b656513463f92808f324f8fcbe34a0b401")
> +  (define revision "1")
>    (package
>      (name "eudev")
> -    (version "3.2.11")
> +    ;; Remove the snippet when upgrading to a released version
> +    (version (git-version "3.2.12" revision commit))
>      (source (origin
>                (method git-fetch)
>                (uri (git-reference (url "https://github.com/gentoo/eudev")
> -                                  (commit (string-append "v" version))))
> +                                  (commit commit)))
>                (file-name (git-file-name name version))
>                (sha256
>                 (base32
> -                "0dzaqwjnl55f69ird57wb6skahc6l7zs1slsrzqqfhww33icp6av"))
> -              (patches (search-patches "eudev-rules-directory.patch"))))
> +                "0rqyzmp8kcnxiy1hq13pr2syp4krnf6q97xwlr0bwcd6x4grbak4"))
> +              (patches (search-patches "eudev-rules-directory.patch"))
> +              (modules '((guix build utils)))
> +              (snippet
> +               #~(begin
> +                   ;; configure.ac uses 3.2.14 as the version number, but it
> +                   ;; is not released yet. Remove the snippet once a later
> +                   ;; version is released.
> +                   (substitute* "configure.ac"
> +                     (("AC_INIT\\(\\[eudev\\],\\[3.2.14\\]")
> +                      "AC_INIT([eudev],[3.2.12]"))))))
>      (build-system gnu-build-system)
>      (arguments
>       (list
> @@ -4285,6 +4298,28 @@ (define-public eudev
>                (substitute* "man/make.sh"
>                  (("/usr/bin/xsltproc")
>                   (search-input-file (or native-inputs inputs) "/bin/xsltproc")))))
> +          (add-before 'bootstrap 'install-in-lib
> +            (lambda _
> +              ;; eudev wants to install its provided hwdb files in /etc, but
> +              ;; we want them in udevlibexecdir.
> +              (copy-file "hwdb/Makefile.am" "hwdb/files.am")
> +              (call-with-output-file "hwdb/Makefile.am"
> +                (lambda (port)
> +                  (format port "udevhwdblibdir = $(udevlibexecdir)/hwdb.d\n")
> +                  (format port "include ./files.am")))
> +              (substitute* "hwdb/files.am"
> +                (("dist_udevhwdb_DATA =")
> +                 "dist_udevhwdblib_DATA ="))
> +              ;; eudev wants to install a template udev.conf into /etc, but we
> +              ;; do not care.
> +              (substitute* "src/udev/Makefile.am"
> +                (("dist_udevconf_DATA =")
> +                 "dist_noinst_DATA ="))
> +              ;; eudev thinks we want to make sure /etc/udev/rules.d exists
> +              ;; when installing - we do not.
> +              (substitute* "rules/Makefile.am"
> +                (("\\$\\(MKDIR_P\\) \\$\\(DESTDIR\\)\\$\\(udevconfdir\\)/rules\\.d")
> +                 "true"))))

That's rather complex.  Could we instead modify udev-service-type to
only union what it needs under /etc/udev and not the undesired files from
this package?

>            (add-after 'install 'move-static-library
>              (lambda _
>                (let ((source (string-append #$output "/lib/libudev.a"))
> @@ -4296,19 +4331,14 @@ (define-public eudev
>                  ;; such that Libtool looks for it in the usual places.
>                  (substitute* (string-append #$output "/lib/libudev.la")
>                    (("old_library=.*")
> -                   "old_library=''\n")))))
> -          (add-after 'install 'build-hwdb
> -            (lambda _
> -              ;; Build OUT/etc/udev/hwdb.bin.  This allows 'lsusb' and
> -              ;; similar tools to display product names.
> -              ;;
> -              ;; XXX: This can't be done when cross-compiling. Find another way
> -              ;; to generate hwdb.bin for cross-built systems.
> -              #$@(if (%current-target-system)
> -                     #~(#t)
> -                     #~((invoke (string-append #$output "/bin/udevadm")
> -                                "hwdb" "--update"))))))
> -       #:configure-flags #~(list "--enable-manpages")))
> +                   "old_library=''\n"))))))
> +      #:configure-flags
> +      #~(list "--enable-manpages"
> +              "--sysconfdir=/etc")))

See comment above about justifying this flag.

> +    (native-search-paths
> +      (list (search-path-specification
> +              (variable "UDEV_HWDB_PATH")
> +              (files '("lib/udev/hwdb.d")))))
>      (native-inputs
>       (list autoconf
>             automake
> diff --git a/gnu/packages/patches/eudev-rules-directory.patch b/gnu/packages/patches/eudev-rules-directory.patch
> index 54fc01c6d5..c4b1cfae39 100644
> --- a/gnu/packages/patches/eudev-rules-directory.patch
> +++ b/gnu/packages/patches/eudev-rules-directory.patch
> @@ -4,9 +4,9 @@ The old udev 182 supported $UDEV_CONFIG_FILE, which in turn allowed
>  the search path to be customized, but eudev no longer has this, hence
>  this hack.
>  
> ---- eudev-3.1.5/src/udev/udev-rules.c	2015-10-13 06:22:14.000000000 +0800
> -+++ eudev-3.1.5/src/udev/udev-rules.c	2015-10-16 20:45:38.491934336 +0800
> -@@ -47,15 +47,11 @@
> +--- a/src/udev/udev-rules.c
> ++++ b/src/udev/udev-rules.c
> +@@ -48,16 +48,11 @@ struct uid_gid {
>           };
>   };
>   
> @@ -20,11 +20,12 @@ this hack.
>  -        "/lib/udev/rules.d",
>  -        "/usr/lib/udev/rules.d",
>  -#endif
> +-        "/usr/local/lib/udev/rules.d",
>  +        NULL,			/* placeholder for $EUDEV_RULES_DIRECTORY */
>           NULL};
>   
>   struct udev_rules {
> -@@ -1704,6 +1700,9 @@
> +@@ -1718,6 +1713,9 @@ struct udev_rules *udev_rules_new(struct udev *udev, int resolve_names) {
>   
>           udev_rules_check_timestamp(rules);

It'd be nicer if eudev's build system allowed to configure the above
without patches hard coding things.

-- 
Thanks,
Maxim