[bug#51838] [PATCH v5 06/45] guix: node-build-system: Refactor patch-dependencies phase.

[Philip McGrath]

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@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