[bug#51838] [PATCH v6 05/41] guix: node-build-system: Add 'delete-dependencies' helper function.

[Philip McGrath]

Hi,

On 12/30/21 12:29, Liliana Marie Prikler wrote:
> Am Donnerstag, dem 30.12.2021 um 02:38 -0500 schrieb Philip McGrath:
> > +(define (delete-dependencies pkg-meta absent-dependencies)
> > +  "Functionally update PKG-META, a json object corresponding to a
> > +'package.json' file, to allow building without the ABSENT-
> > DEPENDENCIES.  To
> > +avoid reintroducing the ABSENT-DEPENDENCIES, only use this procedure
> > after the
> > +'patch-dependencies' phase."
> > +  (define delete-fom-jsobject
> > +    (match-lambda
> > +      (('@ . alist)
> > +       (cons '@ (filter (match-lambda
> > +                          ((k . v)
> > +                           (not (member k absent-dependencies))))
> > +                        alist)))))
> > +  (jsobject-update*
> > +   pkg-meta
> > +   "devDependencies" '(@) delete-fom-jsobject
> > +   "dependencies" '(@) delete-fom-jsobject))
> Given this rather easy definition in terms of our helper functions, I
> think this procedure can do more.  Particularly, I'd argue that we can
> define it as such:
>
> (define* (delete-dependencies dependencies #:key (file "package.json")
>                                (json-keys
>                                 '("dependencies" "devDependencies"))
>    "Remove DEPENDENCIES from JSON_KEYS in FILE."
>    (with-atomic-json-file-replacement ...))
>
> This would in turn make it easier to delete dependencies from #:phases,
> eliminating the need to shorten it to #:absent-dependencies.  WDYT?

I don't think '#:json-keys' would be helpful.

In my view, the high-level purpose of 'delete-dependencies', 
'#:absent-dependencies', or whatever is to gather our collective 
procedural knowledge about how to modify a "package.json" file to build 
a package without some of the dependencies its developers have declared 
and to encode that knowledge in a single, abstracted point of control in 
'node-build-system', so that authors of Node.js package definitions can 
simply specify which declared dependencies are absent and leave it to 
'node-build-system' to act accordingly. (I don't think it matters _why_ 
the dependencies are absent, i.e. whether we don't want the them or 
merely don't have them.)

In our experience so far, the necessary modification does concretely 
amount to "Remove DEPENDENCIES from JSON_KEYS in FILE.", but that is not 
the ultimate purpose of this code, and I think that description, along 
with '#:json-keys', ends up being simultaneously too flexible and too 
restrictive. It is unnecessarily flexible because, if a package author 
ever passes some other value for '#:json-keys', that would seem to 
indicate that there's some procedural knowledge missing from 
'node-build-system', and it should be added there. More significantly, 
it unnecessarily seems to restrict 'delete-dependencies' from taking 
other kinds of actions to handle the absent dependencies, if in the 
future we should discover that there's something we need to do that 
wouldn't amount to just adding another JSON key. It's a little odd to 
give an example of something we might not know, but, for example, I 
could imagine learning that correct handling of absent 
"peerDependencies" could require more involved transformation of the 
structures under "peerDependenciesMeta".

As far as the rest of your suggestion, on the one hand, this:

  (define* (delete-dependencies deps #:key (file "package.json"))
    (with-atomic-json-file-replacement ...))

seems like a fine enhancement, and I could live with it---I'd even 
prefer it, if v6 but not v7 of this patch series can achieve consensus.

On the other hand, at the risk of beating a dead horse, it seems like a 
tiny step from the above to:

  (define* ((delete-dependencies deps #:key (file "package.json")) . _)
    (with-atomic-json-file-replacement ...))

which is just another name for 'make-delete-dependencies-phase', which 
AIUI you had found objectionable. (Apparently that shorthand would need 
(ice-9 curried-definitions).)

Indeed, if we observe that '#:file', similarly to '#:json-keys', will 
never be anything _other_ than "package.json", we could further simplify to:

  (define* ((delete-dependencies deps) . _)
    (with-atomic-json-file-replacement ...))

at which point we've basically re-invented the implementation of patch 
v7 05/41, which basically amounts to:

  (define* (delete-dependencies #:key absent-dependencies)
    (with-atomic-json-file-replacement ...))

In other words, I don't agree that any of these possible changes would 
"eliminat[e] the need to shorten it to #:absent-dependencies",

I still feel that there's something I'm fundamentally not understanding 
about your objections to '#:make-absent-dependencies', which is why, in 
v6, I tried to do exactly as you had proposed:

On 12/20/21 17:00, Liliana Marie Prikler wrote:
> Hi Timothy,
>
> Am Montag, dem 20.12.2021 um 15:15 -0500 schrieb Timothy Sample:
>> Hi Philip,
>>
>> Philip McGrath <philip@philipmcgrath.com> writes:
>>
>>> If we took your final suggestion above, I think we'd have something
>>> like this:
>>>
>>> ```
>>> #:phases
>>> (modify-phases %standard-phases
>>>    (add-after 'unpack 'delete-dependencies
>>>      (make-delete-dependencies-phase '("node-tap"))))
>>> ```
>>
>> I’m perfectly happy with this if it’s a compromise we all can agree on.
>> It is exactly what popped into my imagination when I read Liliana’s
>> suggestion.  I guess the one thing missing is that it would not
>> necessarily be implemented on top of better “package.json”
>> manipulation support.  That said, it doesn’t preclude providing that
>> support if/when the need arises.
> In my personal opinion, we would write that support first and perhaps
> the shorthands later.  I.e.
>
> (add-after 'patch-dependencies 'drop-junk
>    (lambda _
>      (with-atomic-json-replacement "package.json"
>        (lambda (json) (delete-dependencies json '("node-tap"))))))

Certainly I do agree that it would be better to support code more 
concise than that! But I think all of these variations are strictly 
worse than '#:absent-dependencies'. It isn't just that they are more 
typing: the require authors of package definitions to have some (not 
very much, but some) procedural knowledge about _how_ 
'node-build-system' deals with the absence of dependencies, rather than 
with '#:absent-dependencies', declaratively specifying _what_ is to be 
done. For example, as I mentioned in my cover letter at 
<https://issues.guix.gnu.org/51838#257>, even my own code from the 
exchange I just quoted:

>>>    (add-after 'unpack 'delete-dependencies
>>>      (make-delete-dependencies-phase '("node-tap"))))

would be broken in v6, because the implementation of 
'delete-dependencies' assumes that the 'patch-dependencies' phase has 
already been run. I think this is an implementation detail that users of 
'node-build-system' should not be required to know! Indeed, I think that 
would be a good reason to have 'patch-dependencies' handle the absent 
dependencies itself, as previous versions of this series have done, but 
at least making 'delete-dependencies' a phase in 'standard-phases', as 
v7 does, relieves the user of burden of managing the ordering 
requirement manually.

I expect the majority of Guix's Node.js packages will continue for the 
foreseeable future to need the functionality for absent dependencies I 
described at the beginning of this email, so I think we should provide 
it through a mechanism that is as high-level, concise, declarative as 
possible, and ideally one that will facilitate automated code generation 
and static analysis. On each of these criteria, I think 
'#:absent-dependencies' is better than any of the other proposals I've 
heard.

But, as I said, in the interest of compromise and moving forward, I'm 
willing to live with something based on v6 for now if that's what can 
achieve consensus, and then propose '#:absent-dependencies' separately. 
So, if you want me to send a new version with one of these other 
variations, tell me which one, and I'll do it.

I hope my tone isn't coming across the wrong way---I really don't mean 
to be snarky! But I am genuinely struggling to understand the 
significance of the difference between:

>>>    (add-after 'unpack 'delete-dependencies
>>>      (make-delete-dependencies-phase '("node-tap"))))

which I thought you objected to, and the result of what I think you've 
most recently proposed:

    (add-after 'patch-dependencies 'delete-dependencies
      (lambda () (delete-dependencies '("node-tap"))))

which would have avoided my earlier reservations about making the JSON 
representation part of the build system's public API for the first time.

So I'm not feeling very confident in my ability to predict what changes 
would or would not block consensus.

-Philip