From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp10.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms0.migadu.com with LMTPS id yB0wAzFvzmGywgAAgWs5BA (envelope-from ) for ; Fri, 31 Dec 2021 03:47:13 +0100 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp10.migadu.com with LMTPS id MENfNzBvzmFgAQAAG6o9tA (envelope-from ) for ; Fri, 31 Dec 2021 03:47:12 +0100 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id 65DE3D50D for ; Fri, 31 Dec 2021 03:47:12 +0100 (CET) Received: from localhost ([::1]:47138 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1n37wk-0002am-Sl for larch@yhetil.org; Thu, 30 Dec 2021 21:47:10 -0500 Received: from eggs.gnu.org ([209.51.188.92]:59096) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1n37wc-0002Yq-Fg for guix-patches@gnu.org; Thu, 30 Dec 2021 21:47:02 -0500 Received: from debbugs.gnu.org ([209.51.188.43]:43325) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1n37wc-0005A0-3r for guix-patches@gnu.org; Thu, 30 Dec 2021 21:47:02 -0500 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1n37wb-0006wI-TR for guix-patches@gnu.org; Thu, 30 Dec 2021 21:47:01 -0500 X-Loop: help-debbugs@gnu.org Subject: [bug#51838] [PATCH v6 05/41] guix: node-build-system: Add 'delete-dependencies' helper function. Resent-From: Liliana Marie Prikler Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Fri, 31 Dec 2021 02:47:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 51838 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: Philip McGrath , 51838@debbugs.gnu.org Cc: Timothy Sample , Pierre Langlois , Jelle Licht Received: via spool by 51838-submit@debbugs.gnu.org id=B51838.164091881626662 (code B ref 51838); Fri, 31 Dec 2021 02:47:01 +0000 Received: (at 51838) by debbugs.gnu.org; 31 Dec 2021 02:46:56 +0000 Received: from localhost ([127.0.0.1]:54871 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1n37wV-0006vy-9z for submit@debbugs.gnu.org; Thu, 30 Dec 2021 21:46:56 -0500 Received: from mail-wr1-f68.google.com ([209.85.221.68]:44806) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1n37wS-0006vd-6O for 51838@debbugs.gnu.org; Thu, 30 Dec 2021 21:46:53 -0500 Received: by mail-wr1-f68.google.com with SMTP id k18so16980854wrg.11 for <51838@debbugs.gnu.org>; Thu, 30 Dec 2021 18:46:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=message-id:subject:from:to:cc:date:in-reply-to:references :user-agent:mime-version:content-transfer-encoding; bh=FnK0pC1qcXUx8R3eqZI7Xyl3r2sXIU0X0gy83uMZmkI=; b=SfEycYvGQ+y6gIZnlMkRq1LtEVIJlzxDHiE+Bi5TvQMF3IrMCRDwFKaQcp6ehTizST ppbs3BwjJU4mDLbLAEJ+f4DvnY//VQwYDg3u7K3ruG3MxZwS/zKn06OpvJU79YtN5Nq5 9srDTnO9eSegDjFzOwmenOn7bMXsReTlTX8dgGkJsV48bYsDPB+ap0sSWh7KW1Ne6w+9 TI3WedmPgVv9tX7FZTu63M9nC4628bDRxNFL2CCIkptshAZOe4RVBXKDCpC20EpC7uI1 CvEpseSOgWPYYa1RZlNqOS1scPD38oa1xDAp72DubB5XG+bO/5UAsewhkzZM1j5tMhyd O9BA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=FnK0pC1qcXUx8R3eqZI7Xyl3r2sXIU0X0gy83uMZmkI=; b=T66t0ODPnZ1+uQID+vzFFB6I703PwwR7il3TxaHwQDV7Imuz+krWptwA+hdLeuSnLw JNFVefdwm970mbRDVl34FD0wr33TBDAhi2Kn7ln3VuQEYWCX6XRgpkxGNkpPm1sY8lMD eW858tMTcX5wBLVtXMQ9Yxot5s5sU0AL4aWPmwzzYdqohMTRnr/l57SpOc1w2qC3AeFO 0KyS9fHXniTgiIXSGiwn+tqlGeZOD/D3t+q6qfxtM4ifBfQeD8WdtS3Bw4WFG7xAIu6R AOM9dk9cHtXA7J9gUZ/iIg5983HB+Fl7L8sGuM5gdbfxsCC/2qhM7Dpeu2QJkdCzrKt2 faLg== X-Gm-Message-State: AOAM531B2bOARrrR0Ml8NV8mj0OgiNmS8IZUgAX9g8ejJWIBEAA5H5DK 78eMMIMrMKURhcrNT0jYm3M= X-Google-Smtp-Source: ABdhPJyy0rigdEgyfW+f8NY/vcMuLqGcSAg7O/ooGmqZ4xWFjscCOusmn2Lgo7sJ/tvoaxmlxP5pIQ== X-Received: by 2002:adf:ea83:: with SMTP id s3mr28666553wrm.171.1640918805948; Thu, 30 Dec 2021 18:46:45 -0800 (PST) Received: from nijino.fritz.box (85-127-52-93.dsl.dynamic.surfer.at. [85.127.52.93]) by smtp.gmail.com with ESMTPSA id b1sm28462628wrd.92.2021.12.30.18.46.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 30 Dec 2021 18:46:45 -0800 (PST) Message-ID: <5b83ba5e35af7ac956a6d5de41cb98a892863b55.camel@gmail.com> From: Liliana Marie Prikler Date: Fri, 31 Dec 2021 03:46:43 +0100 In-Reply-To: <23eaa7e6-c087-d885-924a-192917758bbf@philipmcgrath.com> References: <082a81964a43ae5f735ad2ca433d0dfe00859c35.camel@gmail.com> <20211230073919.30327-1-philip@philipmcgrath.com> <20211230073919.30327-6-philip@philipmcgrath.com> <7d5dd434d7750123fa32cb623df0463d60d3f82f.camel@gmail.com> <23eaa7e6-c087-d885-924a-192917758bbf@philipmcgrath.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.42.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit 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: , Errors-To: guix-patches-bounces+larch=yhetil.org@gnu.org Sender: "Guix-patches" X-Migadu-Flow: FLOW_IN X-Migadu-Country: US ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1640918832; h=from:from:sender:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding:resent-cc: resent-from:resent-sender:resent-message-id:in-reply-to:in-reply-to: references:references:list-id:list-help:list-unsubscribe: list-subscribe:list-post:dkim-signature; bh=FnK0pC1qcXUx8R3eqZI7Xyl3r2sXIU0X0gy83uMZmkI=; b=H4JuTsRDx8UMJkpebnlSDM3VMvkliHIq2Xzc/CAqEZj6I0f/k7ElSMEZkefmoiRhTn4nFe d4edd3Yeg0V0nQ+AiN6aOAZofsc9tvqiWYaGQGq+nFv8S0/H2/TTnVqPv+RQuIUWWRfnmV HgJLgEPp0DyEXS6IB0RcdNSke/vk8XD+NWajjNq4iOtBSl61L4srBR0O+LaKgOa+Kqm324 WkjFrZpNRsiBUXEfFwO44EEjeNeCy3Ey/ll2jpAKw0WWMUyymxr0/HoirRsBxmcTcOfOBF TwjlE/91F7+rnVl7/oH97uZKtGf1vpnLuaohFQVz7x+ujDsNqzDcas2RGjUNSw== ARC-Seal: i=1; s=key1; d=yhetil.org; t=1640918832; a=rsa-sha256; cv=none; b=Jo1smXRNgcbqXP5D5ZlHp2CWhvzA+NOX/2/1uEHMk0C0W+6uLDwcxzjWzHR2Yv/G/VTTWA +t6UiF4p7Ke2kV05TyarzhYHuNY7s3w5Brer+gx1QVTasHtGFkVamDm/Q5AG34xKUYANW7 D0zbA1B20du81QxfTNxI5VDRQ+KIaRo+d2enJgffkt9DT4czLJZNxxpTChtmaVY9FftLyn bFOBskXJo8Q0nW9utR37TegGGrKU7XFprFjkr5Mz6mxaJ0vmZfO8rtsEvoqfNcgd12YoFK /6bSIkMhfCoO+ktS9wwKdIbUt7PUjL0vpi0k9zIIVkHia57Mjte23HZvErZtmw== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=gmail.com header.s=20210112 header.b=SfEycYvG; dmarc=fail reason="SPF not aligned (relaxed)" header.from=gmail.com (policy=none); spf=pass (aspmx1.migadu.com: domain of "guix-patches-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="guix-patches-bounces+larch=yhetil.org@gnu.org" X-Migadu-Spam-Score: -1.97 Authentication-Results: aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=gmail.com header.s=20210112 header.b=SfEycYvG; dmarc=fail reason="SPF not aligned (relaxed)" header.from=gmail.com (policy=none); spf=pass (aspmx1.migadu.com: domain of "guix-patches-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="guix-patches-bounces+larch=yhetil.org@gnu.org" X-Migadu-Queue-Id: 65DE3D50D X-Spam-Score: -1.97 X-Migadu-Scanner: scn1.migadu.com X-TUID: RZz4zy/PNOJv Hi, Am Donnerstag, dem 30.12.2021 um 20:09 -0500 schrieb Philip McGrath: > Hi, > > On 12/30/21 12:29, Liliana Marie Prikler wrote: > > Am Donnerstag, dem 30.12.2021 um 02:38 -0500 schrieb Philip > > McGrath: > > > +  (define delete-fom-jsobject For the record, I forgot to mention the typo here. It obviously ought to be delete-from-jsobject. > [...] > I don't think '#:json-keys' would be helpful. > > In my view, the high-level purpose of 'delete-dependencies', > '#:absent-dependencies', or whatever is to gather our collective > procedural knowledge about how to modify a "package.json" file to > build a package without some of the dependencies its developers have > declared and to encode that knowledge in a single, abstracted point > of control in 'node-build-system', so that authors of Node.js package > definitions can simply specify which declared dependencies are absent > and leave it to 'node-build-system' to act accordingly. (I don't > think it matters _why_ the dependencies are absent, i.e. whether we > don't want the them or merely don't have them.) For the record, you can delete present dependencies as well, which is one shortcoming in the "absent dependencies" metaphor. As for deleting absent packages automatically without specifying them, one could argue that a shortcoming of this set is that it doesn't provide a way of doing that. I'll take a closer look at the ease-of-use as I walk down your reply. > In our experience so far, the necessary modification does concretely > amount to "Remove DEPENDENCIES from JSON_KEYS in FILE.", but that is > not the ultimate purpose of this code, and I think that description, > along with '#:json-keys', ends up being simultaneously too flexible > and too restrictive.  Admittedly, if you read DEPENDENCIES, JSON_KEYS and FILE as mere variables with no meaning that's bad, but the key here is that DEPENDENCIES is a list of dependencies and JSON_KEYS is a list of json keys that refer to dependency lists. Anything else would be unlawful use. > It is unnecessarily flexible because, if a package author ever passes > some other value for '#:json-keys', that would seem to indicate that > there's some procedural knowledge missing from 'node-build-system', > and it should be added there. The reason I have it as such is that a packager might decide in the future to e.g. only drop a peer dependency, that might share a name with a dev dependency or something along those lines. Along with #:file, it's also supposed to guard against files other than package.json containing a list of dependencies that blows up in our faces, which we could edit with the same primitive. That being said, I do get your point in that it's overly flexible for now in which the point is just to rewrite package.json. > More significantly, it unnecessarily seems to restrict 'delete- > dependencies' from taking other kinds of actions to handle the absent > dependencies, if in the future we should discover that there's > something we need to do that wouldn't amount to just adding another > JSON key. It's a little odd to give an example of something we might > not know, but, for example, I could imagine learning that correct > handling of absent "peerDependencies" could require more involved > transformation of the structures under "peerDependenciesMeta". Do you mean that as in "if a dependency is found under this key, then foo, bar and baz also need to happen"? If so, that would be concerning, but also a good reason to have that abstraction. > As far as the rest of your suggestion, on the one hand, this: > >    (define* (delete-dependencies deps #:key (file "package.json")) >      (with-atomic-json-file-replacement ...)) > > seems like a fine enhancement, and I could live with it---I'd even > prefer it, if v6 but not v7 of this patch series can achieve > consensus. Given that you deleted keys, I don't think there's a good reason to keep around file... or can it be located in a subdirectory? > On the other hand, at the risk of beating a dead horse, it seems like > a tiny step from the above to: > >    (define* ((delete-dependencies deps #:key (file "package.json")) . > _) >      (with-atomic-json-file-replacement ...)) > > which is just another name for 'make-delete-dependencies-phase', > which AIUI you had found objectionable. (Apparently that shorthand > would need (ice-9 curried-definitions).) The reason to not use a phase writer here, is that you could combine it with other stuff in a single phase. E.g. (add-after 'unpack 'remove-foo (lambda _ (delete-dependencies '("foo" "bar" "baz")) (delete-file "baz-loader.js"))) > Indeed, if we observe that '#:file', similarly to '#:json-keys', will > never be anything _other_ than "package.json", we could further > simplify to: > >    (define* ((delete-dependencies deps) . _) >      (with-atomic-json-file-replacement ...)) > > at which point we've basically re-invented the implementation of > patch v7 05/41, which basically amounts to: > >    (define* (delete-dependencies #:key absent-dependencies) >      (with-atomic-json-file-replacement ...)) > > In other words, I don't agree that any of these possible changes > would "eliminat[e] the need to shorten it to #:absent-dependencies", Sorry for the typo. > I still feel that there's something I'm fundamentally not > understanding about your objections to '#:make-absent-dependencies', > which is why, in v6, I tried to do exactly as you had proposed: > > On 12/20/21 17:00, Liliana Marie Prikler wrote: >  > Hi Timothy, >  > >  > Am Montag, dem 20.12.2021 um 15:15 -0500 schrieb Timothy Sample: >  >> Hi Philip, >  >> >  >> Philip McGrath writes: >  >> >  >>> If we took your final suggestion above, I think we'd have > something >  >>> like this: >  >>> >  >>> ``` >  >>> #:phases >  >>> (modify-phases %standard-phases >  >>>    (add-after 'unpack 'delete-dependencies >  >>>      (make-delete-dependencies-phase '("node-tap")))) >  >>> ``` >  >> >  >> I’m perfectly happy with this if it’s a compromise we all can > agree on. >  >> It is exactly what popped into my imagination when I read > Liliana’s >  >> suggestion.  I guess the one thing missing is that it would not >  >> necessarily be implemented on top of better “package.json” >  >> manipulation support.  That said, it doesn’t preclude providing > that >  >> support if/when the need arises. >  > In my personal opinion, we would write that support first and > perhaps >  > the shorthands later.  I.e. >  > >  > (add-after 'patch-dependencies 'drop-junk >  >    (lambda _ >  >      (with-atomic-json-replacement "package.json" >  >        (lambda (json) (delete-dependencies json '("node-tap")))))) To be fair, finding the right sweet spot between being overly verbose and code golfing is difficult. > Certainly I do agree that it would be better to support code more > concise than that! But I think all of these variations are strictly > worse than '#:absent-dependencies'. It isn't just that they are more > typing: the require authors of package definitions to have some (not > very much, but some) procedural knowledge about _how_ 'node-build- > system' deals with the absence of dependencies, rather > than with '#:absent-dependencies', declaratively specifying _what_ is > to be done.  Understanding build systems is for nerds. We here at leftpad.org care about the things that are really important. > For example, as I mentioned in my cover letter at > , even my own code from the > exchange I just quoted: > >  >>>    (add-after 'unpack 'delete-dependencies >  >>>      (make-delete-dependencies-phase '("node-tap")))) > > would be broken in v6, because the implementation of > 'delete-dependencies' assumes that the 'patch-dependencies' phase has > already been run.  I think that is an issue with your patch, however. With mine, you could at least add "peerDependencies" on your own. > I think this is an implementation detail that users of > 'node-build-system' should not be required to know! Indeed, I think > that would be a good reason to have 'patch-dependencies' handle the > absent dependencies itself [...] I think something like that can be arranged. We could make patch- dependencies drop all the dependencies it doesn't know about if we add a "verify-dependencies" phase before it. (Note that I already suggested that once). verify-dependencies would raise an error if a dependency is missing and the user could then decide to drop it or (add-before 'verify-dependencies 'drop-dependencies ...). > I expect the majority of Guix's Node.js packages will continue for > the foreseeable future to need the functionality for absent > dependencies I described at the beginning of this email, so I think > we should provide it through a mechanism that is as high-level, > concise, declarative as possible, and ideally one that will > facilitate automated code generation and static analysis. On each of > these criteria, I think '#:absent-dependencies' is better than any of > the other proposals I've heard. > > But, as I said, in the interest of compromise and moving forward, I'm > willing to live with something based on v6 for now if that's what can > achieve consensus, and then propose '#:absent-dependencies' > separately. So, if you want me to send a new version with one of > these other variations, tell me which one, and I'll do it. I think amending v6 with what we learn from this discussion and the discussion on patch 3 of this series is the way to go. Another thing I forget to mention all the time are regexps. I think it'd be beneficial if delete-dependencies could delete dependencies based on their name matching a regexp rather than a string exactly. This would make some of your lists shorter (e.g. "karma.*"), but there might be a debate on whether to use "^karma.*$" or whether to only consider regexps that match the dependency fully. > I hope my tone isn't coming across the wrong way---I really don't > mean to be snarky! But I am genuinely struggling to understand the > significance of the difference between: > >  >>>    (add-after 'unpack 'delete-dependencies >  >>>      (make-delete-dependencies-phase '("node-tap")))) > > which I thought you objected to, and the result of what I think > you've most recently proposed: > >      (add-after 'patch-dependencies 'delete-dependencies >        (lambda () (delete-dependencies '("node-tap")))) To be clear, there are two things in here which I objected. 1. make-delete-dependencies-phase as a procedure which returns the actual phase instead of writing the lambda out. BTW if I ever wrote (lambda () (delete-dependencies ...)), that was a mistake on my part. As for the reason, scroll up. 2. implementing delete-dependencies in terms of messy ad-hoc json primitives using inner defines rather than reusable ones. The goal was not to create the most unwieldy incantation even though it certainly appears as though it was looking back. Sorry for the misunderstanding. > which would have avoided my earlier reservations about making the > JSON representation part of the build system's public API for the > first time. > > So I'm not feeling very confident in my ability to predict what > changes would or would not block consensus. Adding a gratuitous keyword is an immediate blocker as we've discussed at length ;) Using curried functions is also unlikely going to meaningfully advance the discussion, but there could be a niche application in which they're actually useful that I had not thought about. Apart from that, there is a large room simply for discussions, where it is unclear what form consensus will take before it is shaped. For instance, for patch 3, you had to write a lot of functionality from scratch, which prompted me to suggest that you put them in their own file (which would then be public API, though), but with a little bit of refactoring in terms of SRFI-1 we might make it small enough to keep to npm-build-system until Rust folks demand the same goodies. Phase ordering as you mentioned is also still up to debate. So there's a lot in which you can can add your opinion. If you are doubtful about whether or not a particular change would be good, you could probe different versions you have in your head and ask for opinions on them. E.g. provide two or three implementations of assoc-set and let the "best" be selected. If you only have a single one, you can still ask if something like that would be okay and if you receive a "No" that you don't understand it's okay to ask "what would be the problem, what solution do you propose?" etc. XKCD's "inventing a new standard to cover both use-cases" applies as always too :) Cheers