GNU bug report logs - #51838
[PATCH 00/11] guix: node-build-system: Support compiling add-ons with node-gyp.

Previous Next

Full log

View this message in rfc822 format

From: Philip McGrath <philip <at> philipmcgrath.com>
To: Timothy Sample <samplet <at> ngyro.com>
Cc: 51838 <at> debbugs.gnu.org, Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Subject: [bug#51838] [PATCH 00/11] guix: node-build-system: Support compiling add-ons with node-gyp.
Date: Thu, 2 Dec 2021 16:50:58 -0500

Hi!

On 11/28/21 14:27, Timothy Sample wrote:
> Philip McGrath <philip <at> philipmcgrath.com> writes:
> 
>> -  (define (resolve-dependencies package-meta meta-key)
>> -    (fold (lambda (key+value acc)
>> -            (match key+value
>> -              ('@ acc)
>> -              ((key . value) (acons key (hash-ref index key value) acc))))
>> +  (define (resolve-dependencies meta-alist meta-key)
>> +    (match (assoc-ref meta-alist meta-key)
>> +      (#f
>> +       '())
>> +      (('@ . orig-deps)
>> +       (fold (match-lambda*
>> +               (('@ acc)
>> +                acc)
>> +                (((key . value) acc)
>> +                 (if (member key absent-dependencies)
>> +                     acc
>> +                     (acons key (hash-ref index key value) acc))))
>>             '()
>> -          (or (assoc-ref package-meta meta-key) '())))
>> +          orig-deps))))
> 
> There’s a lot of refactoring going here in addition to change at hand.
> To me, it needlessly obscures the actual change (which is just the check
> for membership in 'absent-dependencies', AFAICS).

I could split it into two commits, if that would be more clear. But see 
below.

>>     (with-atomic-file-replacement "package.json"
>>       (lambda (in out)
>> -      (let ((package-meta (read-json in)))
>> -        (assoc-set! package-meta "dependencies"
>> -                    (append
>> -                     '(@)
>> -                     (resolve-dependencies package-meta "dependencies")
>> -                     (resolve-dependencies package-meta "peerDependencies")))
>> -        (assoc-set! package-meta "devDependencies"
>> -                    (append
>> -                     '(@)
>> -                     (resolve-dependencies package-meta "devDependencies")))
>> +      ;; It is unsafe to rely on 'assoc-set!' to update an
>> +      ;; existing assosciation list variable:
>> +      ;; see 'info "(guile)Adding or Setting Alist Entries"'.
> 
> Good catch!
> 
>> +      (let* ((package-meta (read-json in))
>> +             (alist (match package-meta
>> +                      ((@ . alist) alist)))
> 
> Why do we have to unwrap (and then re-wrap later on) the alist?  It
> would be nice to explain that in the changelog.  Maybe it’s just me, but
> I can’t figure it out!  :)

The (guix build json) module represents a JSON object as a Scheme pair 
with the symbol @ in the car and an alist in the cdr. I'd guess this is 
because, if JSON objects were represented as Scheme alists, it would be 
impossible to distinguish the empty JSON object from the empty JSON 
array. (Racket's json library instead represents JSON objects with 
immutable hash tables, a disjoint type.)

I found it confusing that:

 1. this format does not seem to be documented;

 2. `resolve-dependencies` has the same shape as `assoc-ref`, but
    returns an unwrapped alist where `assoc-ref` would return
    a wrapped pair with @; and

 3. prior to this patch, the implementation of `resolve-dependencies`
    was written as though the symbol @ could appear at arbitrary
    positions in the list, which made it less than obvious that it would
    actually occur exactly once, at the head.

I tried to address the last point when refactoring, but I see that the 
patch I sent made matters extra confusing by leaving in dead code from 
the old approach (maybe due to a rebase conflict I vaguely remember?). I 
should fix that, and probably also explain some of this in comments.

-Philip

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.