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

[Liliana Marie Prikler]

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.

> So, the (guix build json) representation of a "package.json" file
> might look like this:
> 
> ```
> `(@ ("name" . "apple")
>      ("version" . "1.0")
>      ("dependencies". (@ ("banana" . "*")
>                          ("pear" . "*")))
>      ("devDependencies" . (@ ("peach" . "*")
>                              ("orange" . "*")))
>      ("peerDependencies" . (@ ("node-gyp" . "7.x")))
>      ("peerDependenciesMeta" . (@ ("node-gyp" . (@ ("optional" .
> #t)))))
>      ("optionalDependencies" . (@ ("node-gyp" . "7.x"))))
> ```
Note that '("dependencies" . (@ ("banana" . "long") ("pear" . "*"))) is
equal to '("dependencies" @ ("banana" . "long") ("pear" . "juicy")).

> 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

> In particular, `append` applied to two JSON objects does not produce
> a JSON object, even ignoring the problem of duplicate keys.
I think we can ignore that for now, but if it bugs you you can convert
to hash table and back or at least assert that the keys are unique.

> Given that the current code adds "peerDependencies" as additional 
> "dependencies", the choice (as I see it) is between the current 
> approach, in which `resolve-dependencies` returns genuine association
> lists and the `let*` block turns them JSON objects, or changing 
> `resolve-dependencies` to return JSON objects and implementing 
> `json-object-append`, which doesn't seem obviously better, unless it 
> were part of broader changes. 
The return value of resolve-dependencies is okay imo.  It's the calling
convention that is not.

> (As an aside, I am not convinced that the current handling of
> "peerDependencies" is right, but I think reevaluating that behavior
> is out of scope for this patch series, and particularly for this
> patch, in which, as Tim said in 
> <https://issues.guix.gnu.org/51838#234>, my goal was merely to make
> the use of `assoc-set!` safe.)
I agree that we can ignore the semantics of peerDependencies for now
and we may even be able to look aside the use of assoc-set! (although
for the purpose of making it easier for the user to rewrite those files
on their own as I laid out before, it might still make sense to drop
that use regardless, leading by example).  

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

> 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".

If you absolutely require it, though:
(define json-object->alist cdr)
(define (alist->json-object alist) (cons '@ alist))
Magic.

Cheers