From patchwork Fri Oct 6 16:45:46 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Vivien Kraus X-Patchwork-Id: 1271 Return-Path: X-Original-To: patchwork@mira.cbaines.net Delivered-To: patchwork@mira.cbaines.net Received: by mira.cbaines.net (Postfix, from userid 113) id 3475B27BBEA; Fri, 6 Oct 2023 18:32:04 +0100 (BST) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on mira.cbaines.net X-Spam-Level: X-Spam-Status: No, score=-2.7 required=5.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,MAILING_LIST_MULTI,SPF_HELO_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mira.cbaines.net (Postfix) with ESMTPS id 0936227BBE2 for ; Fri, 6 Oct 2023 18:32:02 +0100 (BST) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qoofy-0006lj-LP; Fri, 06 Oct 2023 13:31:53 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qoofw-0006kx-9O for guix-patches@gnu.org; Fri, 06 Oct 2023 13:31:45 -0400 Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qoofw-0006YB-1a for guix-patches@gnu.org; Fri, 06 Oct 2023 13:31:44 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1qoogE-0000WV-Hn for guix-patches@gnu.org; Fri, 06 Oct 2023 13:32:02 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#66099] [PATCH gnome-team v8 0/6] Update eudev, udev-service-type, upower Resent-From: Vivien Kraus Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Fri, 06 Oct 2023 17:32:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 66099 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: Liliana Marie Prikler , Maxim Cournoyer , 66099@debbugs.gnu.org Cc: rg@raghavgururajan.name Received: via spool by 66099-submit@debbugs.gnu.org id=B66099.16966134871905 (code B ref 66099); Fri, 06 Oct 2023 17:32:02 +0000 Received: (at 66099) by debbugs.gnu.org; 6 Oct 2023 17:31:27 +0000 Received: from localhost ([127.0.0.1]:52562 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qoofe-0000Ue-AM for submit@debbugs.gnu.org; Fri, 06 Oct 2023 13:31:27 -0400 Received: from planete-kraus.eu ([89.234.140.182]:43846) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qoofb-0000UU-Km for 66099@debbugs.gnu.org; Fri, 06 Oct 2023 13:31:25 -0400 Received: from planete-kraus.eu (localhost.lan [127.0.0.1]) by planete-kraus.eu (OpenSMTPD) with ESMTP id 6965ecc4; Fri, 6 Oct 2023 17:31:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=planete-kraus.eu; h= message-id:in-reply-to:references:from:date:subject:mime-version :content-type:content-transfer-encoding:to:cc; s=albinoniB; bh=b 32EIDFfT6SULt6oR8ApdhdMSQA=; b=IulFV3NcJsXhy+5i4v3V56ug9ps/m2wPU b2gmGAtM7gqKFFvaCeni1so3nQJgyzKZBuO02qaNVvY0vMqFvwtGWJdogGgR+G5N Yft6hkLwIR4wcxgbtmp9GDGNEMDIs7KMcSybRPBoY0jHfGi9indoBeKDbqc8Pa2V HJoBG3arQBaS/A7GeyTrN793DG/I+k/qlYkI4SsSUvuRZSkTltODqQAdw7uIK2ug aczGkEfxCuFeX0WnoOw+hgtb/THeO4JPtUyLsuEU+k5eC7RXJQqeRHeQWrXjscDy haIiX5blW1Lz7pFJrUCrHG1ShiRs3oO4RFgR24CZdx0yIsFu5kKFA== Received: by planete-kraus.eu (OpenSMTPD) with ESMTPSA id 61cb4693 (TLSv1.3:TLS_CHACHA20_POLY1305_SHA256:256:NO); Fri, 6 Oct 2023 17:31:02 +0000 (UTC) Message-ID: In-Reply-To: <0825bf42daa34ee464c5237b04ad6bf90942e489.camel@gmail.com>, , <1a1089275d13e02b4485c5de6e908ece88f5d2b6.camel@gmail.com><87y1ghryel.fsf@gmail.com>, <87pm1try6q.fsf@gmail.com>, <87lechrvhl.fsf@gmail.com> References: <0825bf42daa34ee464c5237b04ad6bf90942e489.camel@gmail.com> , ,<1a1089275d13e02b4485c5de6e908ece88f5d2b6.camel@gmail.com> <87y1ghryel.fsf@gmail.com>,<87pm1try6q.fsf@gmail.com> ,<87lechrvhl.fsf@gmail.com> Date: Fri, 6 Oct 2023 18:45:46 +0200 MIME-Version: 1.0 User-Agent: Evolution 3.46.4 X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-to: Vivien Kraus X-ACL-Warn: , Vivien Kraus via Guix-patches X-Patchwork-Original-From: Vivien Kraus via Guix-patches via From: Vivien Kraus Errors-To: guix-patches-bounces+patchwork=mira.cbaines.net@gnu.org Sender: guix-patches-bounces+patchwork=mira.cbaines.net@gnu.org X-getmail-retrieved-from-mailbox: Patches 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 /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 /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 > > /etc/udev/hwdb.bin, but > > since the sysconf dir is now directly /etc, the hwdb.bin index will > > not be > > found under /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] : New phase. > > : New phase. > > : 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