From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp0 ([2001:41d0:8:6d80::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms0.migadu.com with LMTPS id IMEpMd9MwWHqXQAAgWs5BA (envelope-from ) for ; Tue, 21 Dec 2021 04:41:19 +0100 Received: from aspmx1.migadu.com ([2001:41d0:8:6d80::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp0 with LMTPS id SNHcLN9MwWFmNgAA1q6Kng (envelope-from ) for ; Tue, 21 Dec 2021 03:41:19 +0000 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 03A2AE09D for ; Tue, 21 Dec 2021 04:41:19 +0100 (CET) Received: from localhost ([::1]:45762 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mzW1d-0000Mx-U8 for larch@yhetil.org; Mon, 20 Dec 2021 22:41:17 -0500 Received: from eggs.gnu.org ([209.51.188.92]:32828) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mzW1O-0000Ml-Tx for guix-patches@gnu.org; Mon, 20 Dec 2021 22:41:03 -0500 Received: from debbugs.gnu.org ([209.51.188.43]:40384) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mzW1O-0005TH-GY for guix-patches@gnu.org; Mon, 20 Dec 2021 22:41:02 -0500 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1mzW1O-00088k-CR for guix-patches@gnu.org; Mon, 20 Dec 2021 22:41:02 -0500 X-Loop: help-debbugs@gnu.org Subject: [bug#51838] [PATCH v5 06/45] guix: node-build-system: Refactor patch-dependencies phase. Resent-From: Philip McGrath Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Tue, 21 Dec 2021 03:41:02 +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: Liliana Marie Prikler , 51838@debbugs.gnu.org Cc: Timothy Sample , Pierre Langlois , Jelle Licht Received: via spool by 51838-submit@debbugs.gnu.org id=B51838.164005802531239 (code B ref 51838); Tue, 21 Dec 2021 03:41:02 +0000 Received: (at 51838) by debbugs.gnu.org; 21 Dec 2021 03:40:25 +0000 Received: from localhost ([127.0.0.1]:51930 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mzW0m-00087m-C5 for submit@debbugs.gnu.org; Mon, 20 Dec 2021 22:40:25 -0500 Received: from mail-ua1-f45.google.com ([209.85.222.45]:43774) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mzW0k-00087Y-9K for 51838@debbugs.gnu.org; Mon, 20 Dec 2021 22:40:23 -0500 Received: by mail-ua1-f45.google.com with SMTP id 107so21302970uaj.10 for <51838@debbugs.gnu.org>; Mon, 20 Dec 2021 19:40:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=philipmcgrath.com; s=google; h=message-id:date:mime-version:user-agent:subject:content-language:to :cc:references:from:in-reply-to:content-transfer-encoding; bh=PFTXslpG5SG+BjIsNSnSLEi4XnMuhr1xHkN1k+BrCNc=; b=N4YlUzvXNmflsd8GdNwQc2ZoSQEwsSgWwrflElJw2wRgt6nM1vX3VqxYnTQ8QC+bns GrW3qhrK8DNBXdTqASd3GPfq3XGRn8SxeUzst2AmsbZQciPIA1VlOSWiPtQtretzH1UB 1rb0AC8upOTcPBcLi+M2CFAgwn8Wv++wD0lvpUpq5Gxm4mJHgy5efcA4f+4lkks+Dxcy Fb16XmSNSFhRkwPg99P0RvKbvSh94M0R/NFRuEWG53gwxMynmPtiJC4khEeMDzIabey8 deNmEzANStZ6xBmx51n06jXKfY4hbhPN39W3TT1cvSED7fRMkkC2phynmkm6StoUu86X 9xrA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=PFTXslpG5SG+BjIsNSnSLEi4XnMuhr1xHkN1k+BrCNc=; b=Dvx5ho0pjeMjzvLh/frocoF+BPqNdRrprZI3TRQ43On4i7+j0CDnZ28y+rG4SQcMkB beTN3AZNzxnyxbeQBukTc0PdCMQrka4H+4RvDwdzlZfimnWuhhB7KuMQjrjjHySw27LH gqNv53T9Zu2fJAFXTIf2iR7x12dVQnKh66GQyouWWN9pYUfovCk3e+Tgt9lG0JRNfLAP oCrPUg6Svz/A+bDkQ85dnrEu2zfn970F/WS+Crj9VUEuuwmMp1escprJ8TZC1j+6hEoI o3XjB1/fM8M/L3Iw2kSpsxp8VavpZKB9MbxR4mI3EjZ0H6snjdgWCFQRwgJx7h5XoFsJ c5xQ== X-Gm-Message-State: AOAM533gNwMMExRkY3Quxvhz+GGjuy/LSM4NRGzVjVzCoUZmAp3Yi6Nk EH7X1ELw2b8c7elPXiUmsgUFzg== X-Google-Smtp-Source: ABdhPJwWf972jfuNUIP9wqEJskzyzQNdXhMcsm9Q2qe72nCKeOwizjRK2jUd3MuPVBM19PU9PoAMwA== X-Received: by 2002:a67:c31e:: with SMTP id r30mr526652vsj.67.1640058016343; Mon, 20 Dec 2021 19:40:16 -0800 (PST) Received: from [192.168.45.37] (c-73-125-89-242.hsd1.fl.comcast.net. [73.125.89.242]) by smtp.gmail.com with ESMTPSA id n21sm2053744vkk.5.2021.12.20.19.40.15 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 20 Dec 2021 19:40:16 -0800 (PST) Message-ID: <45723625-95b6-47e2-4e59-cc5292830520@philipmcgrath.com> Date: Mon, 20 Dec 2021 22:40:15 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.3.1 Content-Language: en-US References: <20211213060107.129223-1-philip@philipmcgrath.com> <20211217020325.520821-1-philip@philipmcgrath.com> <20211217020325.520821-7-philip@philipmcgrath.com> <17f63a58ea9b462e67847f9c7698a119e3915a08.camel@gmail.com> <650d1c43-8106-e7e9-5855-29cfa5f9d147@philipmcgrath.com> <9fe83c79f796bb323816d2c02e45b4277680eda0.camel@gmail.com> <4fead57b-66f0-a0e8-cf3b-a65a8ecba1b2@philipmcgrath.com> <60154db1c83f2f21368fbd3157929f66c916da52.camel@gmail.com> From: Philip McGrath In-Reply-To: <60154db1c83f2f21368fbd3157929f66c916da52.camel@gmail.com> Content-Type: text/plain; charset=UTF-8; format=flowed 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=1640058079; 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=PFTXslpG5SG+BjIsNSnSLEi4XnMuhr1xHkN1k+BrCNc=; b=MDp7QV5pKNOyScbMXtFm533Wklbh+4dKdgfDR8LmJN0Y668YOUpUayIDQ6dGa2waJzeb1+ qAZNU8z93VjHtfssn29EKe7uW8IQAsiMiFDIBeKgB2KF/i0IT2JGWAix89CMosn3bTDbrD i8sySr0u6RQO/bx48mflWe8hfvhWLtGcV7049nITo6lOKRjeP2ZIEZebQ6Ds8FySXaL6pA MqncE5OT1PCdEmp5LL2lKAKCB7zKJXOP4eLPMMLP0PH+mXwY/p8UqrE54rxzH+BN3+AqbK hZeAytTX7QM4kNhkXTYKmj/MY59ZQgJgR4d98wJGCgS/+X9jQwakG/vPRuaVPg== ARC-Seal: i=1; s=key1; d=yhetil.org; t=1640058079; a=rsa-sha256; cv=none; b=RpST6ZvZirjuvocdAzTRkPMwG6R4vwog/ovJNX1ZjMUDunTC8ms+bWhy28sR05tAvoX9wb zevWXz5jJwaRl3a145wvgqPYFd+1UCWKXF9uuzOdumea/MbvAFe6MDFHeC/2+LQdMAMDWN EdAC1vJBU2uaGqkZwdbBArCn/LNSNBatpCZcQvuK37OXZ6eAutZjovP0e+OuQ3CacPLm3z CG3es41WlyN2tLd+ApTxORH8I9B6kgLwpLoloUnFLLiADHRpGQQreAS/AlcC5RKSnfx8sx 8s9HOE275x+tDQARJ+2gElp478P+nN8xhu7fZzb2VE7ByjizE76IRFuymnHdfg== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=philipmcgrath.com header.s=google header.b=N4YlUzvX; dmarc=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.63 Authentication-Results: aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=philipmcgrath.com header.s=google header.b=N4YlUzvX; dmarc=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: 03A2AE09D X-Spam-Score: -1.63 X-Migadu-Scanner: scn0.migadu.com X-TUID: nmVg+6VLKf2V Hi Liliana, On 12/20/21 14:54, Liliana Marie Prikler wrote: > Hi, > > Am Montag, dem 20.12.2021 um 13:03 -0500 schrieb Philip McGrath: >> I definitely am not understanding what you have in mind here. When >> you write "strip the @", I'm not sure what you're referring to, >> because there are multiple "@" tags here, one beginning each JSON >> object. (Maybe this is obvious, but it hadn't been obvious to me.) > I'm referring to this part: >> + (define (resolve-dependencies meta-alist meta-key) >> + ;; Given: >> + ;; - The alist from "package.json", with the '@ unwrapped >> + ;; - A string key, like "dependencies" >> + ;; Returns: an alist (without a wrapping '@) like the entry in >> + ;; meta-alist for meta-key, but with dependencies supplied >> + ;; by Guix packages mapped to the absolute store paths to use. >> + (match (assoc-ref meta-alist meta-key) >> + (#f >> + '()) >> + (('@ . orig-deps) >> + (fold (match-lambda* >> + (((key . value) acc) >> + (acons key (hash-ref index key value) acc))) >> + '() >> + orig-deps)))) > You could simply write the function s.t. (resolve-dependencies DEPS) > takes an alist and then produces an alist of resolved dependencies. > Because you don't, you need to code around that quirk down in the file > replacements. You can safely access the dependencies using > (or (and=> (assoc "dependencies" json) cddr) '()) > in the calling code. If you replace "dependencies" by a generic KEY, > you can also outline that into a helper function. > >> An unfortunate consequence of this representation is that JSON >> objects are not usable directly as association lists: some procedures >> expecting association lists seem to silently ignore the non-pair at >> the head of the list, but I don't think that's guaranteed, and other >> procedures just don't work. > There are sloppy variants of the assoc functions, but again, you are > looking at this from the wrong angle. Just ignore that you have a JSON > object and pass the alist to resolve-dependencies, then reconstruct a > JSON object from the result. ezpz To check my understanding, are you saying you'd like the code to look like this? (n.b. not tested) ``` (define* (patch-dependencies #:key inputs #:allow-other-keys) (define index (index-modules (map cdr inputs))) (define (resolve-dependencies orig-deps) (fold (match-lambda* (((key . value) acc) (acons key (hash-ref index key value) acc))) '() orig-deps)) (define (lookup-deps-alist meta-alist meta-key) (match (assoc-ref meta-alist meta-key) (#f '()) (('@ . deps) deps))) (with-atomic-file-replacement "package.json" (lambda (in out) (let* ((package-meta (read-json in)) (alist (match package-meta ((@ . alist) alist))) (alist (assoc-set! alist "dependencies" (cons '@ (resolve-dependencies (append (lookup-deps-alist alist "dependencies") (lookup-deps-alist alist "peerDependencies")))))) (alist (assoc-set! alist "devDependencies" (cons '@ (resolve-dependencies (lookup-deps-alist alist "devDependencies"))))) (package-meta (cons '@ alist))) (write-json package-meta out)))) #t) ``` I wouldn't have messed with it if I'd found it this way, but to me it does not seem obviously better. In particular, I think any benefit of simplifying `resolve-dependencies` is outweighed by `lookup-deps-alist` conflating looking up the key in the given alist with unwrapping the result, if there is one. Just to be explicit, your much more elegant code with `match-lambda` from above: > (map (match-lambda > (('dependencies '@ . DEPENDENCIES) > (filter away unwanted dependencies)) > (('devDependencies '@ . DEPENDENCIES) > (same)) > (otherwise otherwise)) > json))))) wouldn't quite be enough here: it's possible for the "dependencies" key to be absent but the "peerDependencies" key to be present, in which case, to preserve the current behavior, we still need to add the filtered/rewritten "peerDependencies" as "dependencies". >> But I think those improvements are out of scope for this patch >> series. > I don't. Particularly, the current implementation quirks appear to be > the sole reason you came up with the solution of #:absent-dependencies, > which for the record I still disagree with. We can lay down a better > foundation to rewrite JSON data in node-build-system and should export > functions that are necessary to do so in build-side code if they're not > already part of core guile or other imports. I think the current implementation quirks have almost nothing to do with my reasoning for #:absent-dependencies. It's probably true that, if it were more convenient to write phases transforming "package.json" generally, I probably wouldn't have looked into why so many existing packages were deleting the configure phase---but I think it's probably also true that, if that had been the case, those packages wouldn't currently be deleting the configure phase. Part of my point is that, even if those utility functions did exist, I would still advocate for #:absent-dependencies. Jelle's latest email covers much of my reasoning, particularly this: On 12/20/21 18:10, Jelle Licht wrote: > Liliana Marie Prikler writes: >> That is the point, but please don't add a function called "make-delete- >> dependencies-phase". We have lambda. We can easily add with-atomic- >> json-replacement. We can also add a "delete-dependencies" function >> that takes a json and a list of dependencies if you so want. >> >> So in short >> >> (add-after 'patch-dependencies 'drop-junk >> (lambda _ >> (with-atomic-json-replacement "package.json" >> (lambda (json) (delete-dependencies json '("node-tap")))))) >> >> would be the "verbose" style of disabling a list of dependencies. >> > > I think you are _really_ underestimating how many packages will need a > phase like this in the future. I would agree with this approach if it > were only needed incidentally, similar to the frequency of patching > version requirements in setup.py or requirements.txt for python > packages. > > Pretty much everything except the '("node-tap") list will be identical > between packages; how do you propose we reduce this duplication? At this > point I feel like I'm rehasing the opposite of your last point, so let > me rephrase; how many times do you want to see/type/copy+paste the above > snippet before you would consider exposing this functionality on a > higher level? On a similar note, as Jelle said elsewhere, I think #:absent-dependencies should be more amenable to programmatic code generation and transformation than a phase under `modify-phases` would be. The downside of having lambda, of course, is that some analysis is intractable or undecidable on Turing-complete (Church-complete?) code: there is a tradeoff to expressive power. > >> It seems like much more discussion would be needed on what the >> improvements should be, and potentially coordination with other users >> of (guix build json). Personally, I'd want to represent JSON objects >> with a real immutable dictionary type that gave us more guarantees >> about correctness by construction. If we continue with tagged >> association lists, we should write a little library of purely >> functional operations on JSON objects. But that all seems very far >> afield. > I'll have to say non sequitur to that. The functionality we require to > efficiently rewrite JSON can perfectly be built on top of (a)list > primitives and pattern matching, both of which are available to be used > in build-side code. We could even throw in SXML if we needed, not that > we do. There is really no need to code up yet another set of JSON > primitives just to write "hello, world". The other part of my point is that I think providing a nice set of utilities for more general JSON transformation is important enough that it should not be thrown into this patch series as an afterthought. The design is the hard part, not the code. If it's useful as an illustration (maybe it's not ...), here's a non-quirky implementation (not tested) of the interesting parts of `patch-dependencies` in Racket, with (other than the actual IO) exclusively pure functions operating on immutable data structures, which would not eliminate the benefits of `#:absent-dependencies`: ``` (λ (index absent-dependencies in out) (define (resolve-dependencies orig-deps) (for/hasheq ([{k v} (in-immutable-hash orig-deps)] #:unless (memq k absent-dependencies)) (values k (hash-ref index k v)))) (define (resolve-package-meta package-meta) (hash-update (hash-update package-meta 'devDependencies resolve-dependencies #hasheq()) 'dependencies (λ (orig-deps) (resolve-dependencies (hash-union orig-deps (hash-ref package-meta 'peerDependencies #hasheq())))) #hasheq())) (write-json (resolve-package-meta (read-json in)) out)) ``` To have a similarly pleasant API---I think we could aspire to an even better one!---the absolute minimum we'd need is a non-destructive `assoc-set`, since apparently that doesn't exist: it is, admittedly, fairly trivial. To be really convenient, though, we'd probably want `assoc-update`. But should `assoc-update` have an implicit default of `#f`, like `assoc-ref`, or raising an error, like Racket's `hash-update`/`hash-ref`/`dict-update`/`dict-ref`? Should we borrow the Racket convention whereby the "default value" can be a thunk (in which case it is called, so it could escape or build up some expensive-to-compute default value)? Or maybe `assoc-update` should take four mandatory arguments, with the update procedure last, because it will often be a long lambda expression, and the code might look more beautiful with that argument last. Maybe the default should be passed as a keyword argument! Then there's `assoc-set*`, `assoc-has-key?`, `assoc-delete`, `assoc-union` ... But wait! A big source of repetitive boilerplate is wrapping and unwrapping JSON objects with the '@ tag. If we're implementing all of these functions anyway, why not effectively compose them with: > > If you absolutely require it, though: > (define json-object->alist cdr) > (define (alist->json-object alist) (cons '@ alist)) so we can just write `jsobject-ref`, `jsobject-set`, `jsobject-delete`, `jsobject-update`, etc. directly? Insert bikeshedding about names here. But wait! Today, (guix build json) is not part of node-build-system's public API. It is not even part of the default value for #:modules (though it is in #:imported-modules). If we are ever going to change the JSON representation we use, surely we should consider it before making it public. There is a commit [1] in the history changing to use guile-json: do the reasons it was reverted "for now" [2] in 2019 still apply? And guile-json is a deprecated alias: would we want guile-json-1, guile-json-3, or guile-json-4? [1]: https://git.savannah.gnu.org/cgit/guix.git/commit/?id=8eb0ba532ebbebef23180e666e0607ea735f9c1a [2]: https://git.savannah.gnu.org/cgit/guix.git/commit/?id=a4bb18921099b2ec8c1699e08a73ca0fa78d0486 ... and so forth. These questions do not strike me as trivially self-evident. I don't know what answers I'd come up with for all of them. Given that, even if we already had these utility functions, I still think #:absent-dependencies would be The Right Thing, I'm very reluctant to add a prerequisite of designing general "package.json" manipulation tools. -Philip