Package: guix-patches;
Reported by: Philip McGrath <philip <at> philipmcgrath.com>
Date: Sun, 14 Nov 2021 12:43:01 UTC
Severity: normal
Tags: patch
Done: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Bug is archived. No further changes may be made.
Message #800 received at 51838 <at> debbugs.gnu.org (full text, mbox):
From: Philip McGrath <philip <at> philipmcgrath.com> To: Liliana Marie Prikler <liliana.prikler <at> gmail.com>, 51838 <at> debbugs.gnu.org Cc: Timothy Sample <samplet <at> ngyro.com>, Pierre Langlois <pierre.langlois <at> gmx.com>, Jelle Licht <jlicht <at> fsfe.org> Subject: Re: [PATCH v5 06/45] guix: node-build-system: Refactor patch-dependencies phase. Date: Mon, 20 Dec 2021 22:40:15 -0500
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 <liliana.prikler <at> gmail.com> 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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.