Message ID | 43e937e1625b47a80887e68847fb8a8811d3f39f.1670867103.git.mirai@makinata.eu |
---|---|
State | New |
Headers |
Return-Path: <guix-patches-bounces+patchwork=mira.cbaines.net@gnu.org> X-Original-To: patchwork@mira.cbaines.net Delivered-To: patchwork@mira.cbaines.net Received: by mira.cbaines.net (Postfix, from userid 113) id 526BF27BBEC; Mon, 12 Dec 2022 17:47:27 +0000 (GMT) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on mira.cbaines.net X-Spam-Level: X-Spam-Status: No, score=-3.9 required=5.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_MSPIKE_H2,SPF_HELO_PASS 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 5BA4927BBE9 for <patchwork@mira.cbaines.net>; Mon, 12 Dec 2022 17:47:24 +0000 (GMT) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from <guix-patches-bounces@gnu.org>) id 1p4mtM-0008Cv-FS; Mon, 12 Dec 2022 12:47:04 -0500 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 <Debian-debbugs@debbugs.gnu.org>) id 1p4mtK-0008CR-5J for guix-patches@gnu.org; Mon, 12 Dec 2022 12:47:02 -0500 Received: from debbugs.gnu.org ([209.51.188.43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from <Debian-debbugs@debbugs.gnu.org>) id 1p4mtJ-0001zj-Td for guix-patches@gnu.org; Mon, 12 Dec 2022 12:47:01 -0500 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from <Debian-debbugs@debbugs.gnu.org>) id 1p4mtJ-0001Zi-Pc for guix-patches@gnu.org; Mon, 12 Dec 2022 12:47:01 -0500 X-Loop: help-debbugs@gnu.org Subject: [bug#60014] [PATCH] activation: make install-special-file match against pairs as well. Resent-From: mirai@makinata.eu Original-Sender: "Debbugs-submit" <debbugs-submit-bounces@debbugs.gnu.org> Resent-CC: guix-patches@gnu.org Resent-Date: Mon, 12 Dec 2022 17:47:01 +0000 Resent-Message-ID: <handler.60014.B.16708671846037@debbugs.gnu.org> Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: report 60014 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: 60014@debbugs.gnu.org Cc: Bruno Victal <mirai@makinata.eu> X-Debbugs-Original-To: guix-patches@gnu.org Received: via spool by submit@debbugs.gnu.org id=B.16708671846037 (code B ref -1); Mon, 12 Dec 2022 17:47:01 +0000 Received: (at submit) by debbugs.gnu.org; 12 Dec 2022 17:46:24 +0000 Received: from localhost ([127.0.0.1]:54358 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from <debbugs-submit-bounces@debbugs.gnu.org>) id 1p4msh-0001ZI-UK for submit@debbugs.gnu.org; Mon, 12 Dec 2022 12:46:24 -0500 Received: from lists.gnu.org ([209.51.188.17]:40022) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from <mirai@makinata.eu>) id 1p4msf-0001ZB-30 for submit@debbugs.gnu.org; Mon, 12 Dec 2022 12:46:22 -0500 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 <mirai@makinata.eu>) id 1p4mse-000887-RB for guix-patches@gnu.org; Mon, 12 Dec 2022 12:46:20 -0500 Received: from smtpmciv5.myservices.hosting ([185.26.107.241]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from <mirai@makinata.eu>) id 1p4msc-0001X0-BC for guix-patches@gnu.org; Mon, 12 Dec 2022 12:46:19 -0500 Received: from mail1.netim.hosting (unknown [185.26.106.172]) by smtpmciv5.myservices.hosting (Postfix) with ESMTP id 605C820491 for <guix-patches@gnu.org>; Mon, 12 Dec 2022 18:46:05 +0100 (CET) Received: from localhost (localhost [127.0.0.1]) by mail1.netim.hosting (Postfix) with ESMTP id 1349280096; Mon, 12 Dec 2022 18:46:05 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at mail1.netim.hosting Received: from mail1.netim.hosting ([127.0.0.1]) by localhost (mail1-1.netim.hosting [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id tLSuHF4fyJbW; Mon, 12 Dec 2022 18:46:04 +0100 (CET) Received: from guix-nuc.home.arpa (bl12-93-156.dsl.telepac.pt [85.245.93.156]) (Authenticated sender: lumen@makinata.eu) by mail1.netim.hosting (Postfix) with ESMTPSA id 6D53580093; Mon, 12 Dec 2022 18:46:04 +0100 (CET) From: mirai@makinata.eu Date: Mon, 12 Dec 2022 17:45:47 +0000 Message-Id: <43e937e1625b47a80887e68847fb8a8811d3f39f.1670867103.git.mirai@makinata.eu> X-Mailer: git-send-email 2.38.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Received-SPF: pass client-ip=185.26.107.241; envelope-from=mirai@makinata.eu; helo=smtpmciv5.myservices.hosting X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: guix-patches@gnu.org List-Id: <guix-patches.gnu.org> List-Unsubscribe: <https://lists.gnu.org/mailman/options/guix-patches>, <mailto:guix-patches-request@gnu.org?subject=unsubscribe> List-Archive: <https://lists.gnu.org/archive/html/guix-patches> List-Post: <mailto:guix-patches@gnu.org> List-Help: <mailto:guix-patches-request@gnu.org?subject=help> List-Subscribe: <https://lists.gnu.org/mailman/listinfo/guix-patches>, <mailto:guix-patches-request@gnu.org?subject=subscribe> 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 |
Series |
[bug#60014] activation: make install-special-file match against pairs as well.
|
|
Commit Message
Bruno Victal
Dec. 12, 2022, 5:45 p.m. UTC
From: Bruno Victal <mirai@makinata.eu>
special-files is a list of 2-tuples (pairs) but matching against
a non-list pair would fail as match-lambda was only matching
against a list pattern.
---
gnu/build/activation.scm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
base-commit: 5fb5af5658b7575a945579a7cf51c193600b76bb
Comments
Hi Bruno, Is this patch related to some specific problem you're running into? I personally would prefer keeping the special file interface as-is, and not mix two different kinds of entries: lists with 2 elements, and pairs. That would avoid having to manage even more edge-cases down the line if some more processing is needed. Otherwise, you're missing the ChangeLog entry format for the commit message, which you can find described at [1]. You can take some inspiration from other commits in the repository. Best,
Josselin Poiret via Guix-patches via 写道: > I personally would prefer keeping the special file interface > as-is, and > not mix two different kinds of entries: lists with 2 elements, > and > pairs. That would avoid having to manage even more edge-cases > down the > line if some more processing is needed. I agree with this reasoning, and would go as far as to say that if this fixes anything, that thing should probably be fixed instead…? ‘Takes a list of As, but as a special case, a single A’ is confusing and makes it that much harder for newcomers to move beyond cargo-culting magical snippets. Kind regards, T G-R
On 2022-12-12 20:34, Josselin Poiret wrote: > Hi Bruno, > > Is this patch related to some specific problem you're running into? I > personally would prefer keeping the special file interface as-is, and > not mix two different kinds of entries: lists with 2 elements, and > pairs. That would avoid having to manage even more edge-cases down the > line if some more processing is needed. I'm writing a service definition that uses a special-files-service-type service-extension. The documentation for it says: --8<---------------cut here---------------start------------->8--- The value associated with special-files-service-type services must be a list of tuples where the first element is the “special file” and the second element is its target. --8<---------------cut here---------------end--------------->8--- I assume a pair is a reasonable interpretation of 'tuples' in this context, so I proceeded to serialize the fields with: --8<---------------cut here---------------start------------->8--- (cons "filename here" (mixed-text-file "filename" contents ...)) --8<---------------cut here---------------end--------------->8--- Which I think is the natural way of doing it. (and communicates the intent, a pair with a path and a file-like object.) Of course, (list "path" file-like-obj) works as well but imo the pair is clearer in purpose. (what meaning would the third element and so on have, if ever present?) This I found out the hard way by getting strange errors until I looked into what happens behind `special-files-service-type' and realizing that only lists were accepted. The mixing of cases is unfortunate (it should have been pairs from the start) but preserves compatibility with existing syntax.
On 2022-12-12 20:52, Tobias Geerinckx-Rice wrote: > Josselin Poiret via Guix-patches via 写道: >> I personally would prefer keeping the special file interface as-is, and >> not mix two different kinds of entries: lists with 2 elements, and >> pairs. That would avoid having to manage even more edge-cases down the >> line if some more processing is needed. > > I agree with this reasoning, and would go as far as to say that if this fixes anything, that thing should probably be fixed instead…? > > ‘Takes a list of As, but as a special case, a single A’ is confusing and makes it that much harder for newcomers to move beyond cargo-culting magical snippets. That's not what's happening here, right now what guix does is: take a list of tuples, where tuples are 2-element lists of path + file-like. This patch does: take a list of tuples, where tuples are pairs of path + file-like (and as a bonus, preserve existing configurations by allowing the pairs to be lists as well).
Hi Bruno, mirai <mirai@makinata.eu> writes: > The documentation for it says: > --8<---------------cut here---------------start------------->8--- > The value associated with special-files-service-type services must be a list of tuples where the first element is the “special file” and the second element is its target. > --8<---------------cut here---------------end--------------->8--- > > Which I think is the natural way of doing it. (and communicates the intent, a pair with a path and a file-like object.) Right, that's unfortunate, although that could be changed to “list of lists” to make it clearer. > Of course, (list "path" file-like-obj) works as well but imo the pair is clearer in purpose. > (what meaning would the third element and so on have, if ever present?) > This I found out the hard way by getting strange errors until I looked into what happens behind > `special-files-service-type' and realizing that only lists were accepted. > > The mixing of cases is unfortunate (it should have been pairs from the start) but preserves > compatibility with existing syntax. I agree with you here, but then I think to avoid having to maintain both cases at the same time, all existing uses of special-files-service-type should also be modified, and only one kind should remain, with the other triggering some deprecation warning. You could match to `(path . file-like)`, and if (list? file-like), throw an exception. As a sidenote, the main problem is that Guile is not a statically typed language, but that's a whole other debate to have. In any case, I don't think this patch will be accepted as-is. I would only be partially in favor of the second solution (because it breaks existing code), while the first solution is low-effort and should work well enough. Up to you (and maintainers) to decide. Best,
On 2022-12-13 10:15, Josselin Poiret wrote: > Hi Bruno, > > mirai <mirai@makinata.eu> writes: > >> The documentation for it says: >> --8<---------------cut here---------------start------------->8--- >> The value associated with special-files-service-type services must be a list of tuples where the first element is the “special file” and the second element is its target. >> --8<---------------cut here---------------end--------------->8--- >> >> Which I think is the natural way of doing it. (and communicates the intent, a pair with a path and a file-like object.) > > Right, that's unfortunate, although that could be changed to “list of > lists” to make it clearer. > >> Of course, (list "path" file-like-obj) works as well but imo the pair is clearer in purpose. >> (what meaning would the third element and so on have, if ever present?) >> This I found out the hard way by getting strange errors until I looked into what happens behind >> `special-files-service-type' and realizing that only lists were accepted. >> >> The mixing of cases is unfortunate (it should have been pairs from the start) but preserves >> compatibility with existing syntax. > > I agree with you here, but then I think to avoid having to maintain both > cases at the same time, all existing uses of special-files-service-type > should also be modified, and only one kind should remain, with the other > triggering some deprecation warning. You could match to `(path > . file-like)`, and if (list? file-like), throw an exception. The `(= car target) (= cdr file)' match pattern is lifted from https://git.savannah.gnu.org/cgit/guile.git/tree/module/ice-9/match.upstream.scm?id=b54263dc98b2700fa777745405ad7651601bcdc6#n139 as Guile's Pattern Matching page doesn't specify how to match against pairs when I was looking into it. > As a sidenote, the main problem is that Guile is not a statically typed > language, but that's a whole other debate to have. > > In any case, I don't think this patch will be accepted as-is. I would > only be partially in favor of the second solution (because it breaks > existing code), while the first solution is low-effort and should work > well enough. Up to you (and maintainers) to decide. A breaking change here (or a non-breaking "deprecated" warning similar to how bootloader target field was renamed to targets can be done too, but before any further changes its best to discuss if such a change will be received. On 2022-12-12 20:34, Josselin Poiret wrote: > Otherwise, you're missing the ChangeLog entry format for the commit > message, which you can find described at [1]. You can take some > inspiration from other commits in the repository. I'm missing the link at [1], could you resend it? Cheers, Bruno
mirai <mirai@makinata.eu> writes:
> I'm missing the link at [1], could you resend it?
My bad, here it is
[1] https://www.gnu.org/prep/standards/html_node/Change-Logs.html#Change-Logs
Heyo, mirai 写道: > This patch does: take a list of tuples, where tuples are pairs > of path + file-like This is fine. > (and as a bonus, > preserve existing configurations by allowing the pairs to be > lists as well). This not so much. I guess my example was poorly chosen, but at least deprecate the old style, as jpoiret also suggests. That does not mean you need to instantly break old configurations. Kind regards, T G-R
Hi, Josselin Poiret <dev@jpoiret.xyz> skribis: > Is this patch related to some specific problem you're running into? I > personally would prefer keeping the special file interface as-is, and > not mix two different kinds of entries: lists with 2 elements, and > pairs. That would avoid having to manage even more edge-cases down the > line if some more processing is needed. I agree. This is a public-facing interface so we should keep it as-is; extending it to support pairs in addition to two-list elements would likely bring confusion and bugs. I’m not entirely sure why we settled on two-list elements rather than pairs back then, but I think it’s OK. Closing? Ludo’.
Hi, While thinking about this, I've noticed that using lists as "pairs" is a pattern that is common in the existing guix code, with openssh-service-type 'authorized-keys' field and G-Expressions 'file-union' as examples. Given the "entrenched" list usage, I don't think it's worth the effort to change the whole system to use pairs at this point (or maybe allow it as it probably just creates more confusion). I will amend the special-files-service-type doc entry to clarify that it expects two-element lists instead. Bruno On 2022-12-20 14:47, Ludovic Courtès wrote: > Hi, > > Josselin Poiret <dev@jpoiret.xyz> skribis: > >> Is this patch related to some specific problem you're running into? I >> personally would prefer keeping the special file interface as-is, and >> not mix two different kinds of entries: lists with 2 elements, and >> pairs. That would avoid having to manage even more edge-cases down the >> line if some more processing is needed. > > I agree. This is a public-facing interface so we should keep it as-is; > extending it to support pairs in addition to two-list elements would > likely bring confusion and bugs. > > I’m not entirely sure why we settled on two-list elements rather than > pairs back then, but I think it’s OK. > > Closing? > > Ludo’.
diff --git a/gnu/build/activation.scm b/gnu/build/activation.scm index 10c9045740..d4a7559651 100644 --- a/gnu/build/activation.scm +++ b/gnu/build/activation.scm @@ -341,7 +341,7 @@ (define (activate-special-files special-files) " (define install-special-file (match-lambda - ((target file) + ((or (target file) (? pair? (= car target) (= cdr file))) (let ((pivot (string-append target ".new"))) (mkdir-p (dirname target)) (symlink file pivot)