[bug#66099] [PATCH gnome-team v8 0/6] Update eudev, udev-service-type, upower

[Vivien Kraus via Guix-patches via <guix-patches@gnu.org>]

Dear guix,

Thank you for your reviews! Since eudev released 3.2.14 in the mean time, I
udpated it. I also reused more code in the udev-service-type helper functions.

Le jeudi 05 octobre 2023 à 07:42 +0200, Liliana Marie Prikler a écrit :
> Am Montag, dem 02.10.2023 um 21:08 +0200 schrieb Vivien Kraus:
> > The "rules" field in the udev-configuration record can now hold
> > both
> > rules and hwdb files.
> Yeah, how about we instead have separate fields – one named "rules" and one
> named "hardware".  We're already breaking ABI stability anyway, so we might
> as well.

Okay. I fixed the udev-service-type function so that both fields are populated
correctly.

> Speaking of which, we might want to rename "hwdb.d" to "hardware" in udev
> itself to make that name readable, but keep /etc/udev/hwdb.bin as-is.

I renamed to "hardware" in Guix documentation, but individual packages (if
they behave like upower) expect to install their hardware files in "hwdb.d",
so I think we should keep "hwdb.d" as the directory name.

Le jeudi 05 octobre 2023 à 08:53 +0200, Liliana Marie Prikler a écrit :
> Am Donnerstag, dem 05.10.2023 um 07:55 +0200 schrieb Vivien Kraus:
> > Le jeudi 05 octobre 2023 à 07:42 +0200, Liliana Marie Prikler a
> > écrit :
> > > Am Montag, dem 02.10.2023 um 21:08 +0200 schrieb Vivien Kraus:
> > > > The "rules" field in the udev-configuration record can now hold
> > > > both rules and hwdb files.
> > > Yeah, how about we instead have separate fields – one named
> > > "rules"
> > > and one named "hardware".
> > 
> > Since the extensions are untyped anyway, I thought we would not
> > care
> > in the configuration record itself. Should we also introduce types
> > for the extension values?
> This is about field semantics rather than type theory, but if you
> prefer, I think we can use list-of file-like? or similar for both.

I addressed the "field semantics" view of the problem.

Le jeudi 05 octobre 2023 à 09:28 -0400, Maxim Cournoyer a écrit :
> > 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.

As discussed in IRC, we set sysconfdir to /etc so that it can have hardware
files from many packages. So the hardware files are searched from either
"/etc/udev/hwdb.d" or "$individual_package_prefix/lib/udev/hwdb.d". So, the
only standard directory to install hwdb files during a build of a package is
$package_prefix/lib/udev/hwdb.d.

Le jeudi 05 octobre 2023 à 09:28 -0400, Maxim Cournoyer a écrit :
> > 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.

I added comments in the code. As mentioned in IRC, sysconfdir defaults to
$prefix/etc. If we use $eudev_prefix/etc/udev/hwdb.d as the directory where
the highest priority hardware files are found, then we won’t be able to add
files to it at all, and it will be disconnected from /etc/udev/hwdb.d
(prepared by the udev-service-type).

Le jeudi 05 octobre 2023 à 09:28 -0400, Maxim Cournoyer a écrit :
> > +          (add-before 'bootstrap 'install-in-lib
> > +            (lambda _
[...]
> 
> 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?

Considering the steps in this phase other than changing the installation
directory of hardware files, the build system wants to install things to /etc,
which is unaccessible when building the package. So we either have to change
the installation directory of the empty template configuration, or disable the
installation. Same for the mkdir -p, we either have to make the empty
directory in the installation prefix of the package, or disable it. I think
that the empty configuration file and the empty directory are not worth
keeping.

> > +              "--sysconfdir=/etc")))
> 
> See comment above about justifying this flag.

As discussed, we need it. The added comments should help justify it.

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

Indeed. For now, I think the patch is fine (I know very little about udev
rules though).

Le jeudi 05 octobre 2023 à 09:33 -0400, Maxim Cournoyer a écrit :
> I'm not sure using Texinfo in plain docstrings provides much; there's no
> tooling to render them and it's a convention to use FULL CAPS for variable
> names in Guile doc strings, at least in the Guix code base.

(discussing the docstrings use of texinfo)

Le jeudi 05 octobre 2023 à 10:31 -0400, Maxim Cournoyer a écrit :
> > Some udev-related auxiliary functions in (gnu services base) had
> > non-texinfo
> > variable references in their docstrings ("FILE-NAME" instead of
> > "@var{file-name}").
>
> That's fairly conventional.  Texinfo is used in the manual documentation,
> package synopsis/descriptions and services documentation, not for every doc
> string in the code base.

Since the ,describe top-level guile command does not try to render texinfo, I
think you are right. I reverted the changes.

I changed the documentation as indicated ("matter", double spaces, "@file",
the udev-rule introduction, "@code{operating-system}", the android-udev-rules
comment).

> As discussed above, I'm not convinced about changing dostrings to use
> Texinfo, but I see that was already the cases for some around.  Hm.

I guess that’s from guile people (in guile, the docstrings are extracted to be
included in the manual).

About the texinfocation of the udev-rules-service docstring:
> I'd drop this change.

Done.

> > +@item @code{native-udev} (default: @code{eudev}) (type: file-like)
> > +Native udev package to compile @code{hwdb.bin}. The trie format used for
> > +@code{hwdb.bin} must be compatible with the @code{udev} and
> > +@code{native-udev} fields of the udev configuration. To avoid generating
> > +@code{hwdb.bin}, pass @code{#f} as the @code{native-udev} field.
> > +
> > +In any case, if the package version string differs between @code{udev}
> > +and @code{native-udev}, @code{hwdb.bin} is @strong{not} created.
> 
> Shouldn't that raise an error with a useful error message?  Then it doesn't
> need to be documented here.  Thinking again, why is it necessary to have an
> explicite native-udev field?  The gexps mechanism of the service could
> perhaps use #+udev instead of #$udev where needed, sharing the same 'udev'
> field for both purposes?

Further down, about the native-udev file in the configuration record definition:
> As discussed earlier, I don't think that new field is needed.

And further down:
> > +                        (invoke #+(file-append native-udev "/bin/udevadm")
> > [...]
>
> As discussed above, this can probably be simplified by dropping native-udev.
> If it's truly needed, the pathological case where different versions are
> needed should be handled earliest and an error raised with a useful message.

I dropped the native-udev field, with a warning that it will be used both in
the current system and the target system.

> Could you please send a v8 with changes along those suggested?
Here!

Best regards,

Vivien

Vivien Kraus (6):
  gnu: eudev: Update to 3.2.14.
  services: udev: Rewrite udev-rule to use file->udev-rule.
  services: udev: Make udev-rule helper functions generic.
  gnu: udev-service-type: accept hardware description file extensions.
  gnu: libgudev: Update to 238.
  gnu: upower: Update to 1.90.2.

 doc/guix.texi                                 |  57 ++++++--
 gnu/packages/gnome.scm                        |  43 +++---
 gnu/packages/linux.scm                        |  57 +++++---
 .../patches/eudev-rules-directory.patch       |   9 +-
 gnu/services/base.scm                         | 124 +++++++++++++-----
 5 files changed, 205 insertions(+), 85 deletions(-)


base-commit: b18b2d13488f2a92331ccad2dc8cbb54ee15582f
-- 
2.41.0