Package: guix-patches;
Reported by: Vivien Kraus <vivien <at> planete-kraus.eu>
Date: Tue, 19 Sep 2023 11:40:01 UTC
Severity: normal
Tags: patch
Done: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Vivien Kraus <vivien <at> planete-kraus.eu> To: Liliana Marie Prikler <liliana.prikler <at> gmail.com>,Maxim Cournoyer <maxim.cournoyer <at> gmail.com>,66099 <at> debbugs.gnu.org Cc: rg <at> raghavgururajan.name Subject: [bug#66099] [PATCH gnome-team v8 0/6] Update eudev, udev-service-type, upower Date: Fri, 6 Oct 2023 18:45:46 +0200
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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.