[bug#51838] [PATCH v8 00/41] guix: node-build-system: Support compiling add-ons with node-gyp.

[Philip McGrath]

Hi,

On 1/6/22 12:45, Liliana Marie Prikler wrote:
> Hi Philip,
>
> Please pardon the large flood of messages in a short enough of time
> without waiting for a reply.  I tried my best to come up with a v8 that
> addresses my personal concerns quickly.
>
> My main changes were to patches 3-5, but note how deleting dependencies
> became significantly easier in 6pp.  To list the main differences:
> 1. I've implemented the alist/JSON utilities in terms of SRFI-1 and SRFI- > 2. I shortened the additional JSON API to jsobject-ref and 
jsobject-update*.
>     I still believe alist->json-object and json-object->alist would be more
>     useful (in particular, the main operation of jsobject-union is actually
>     alist-flatten), but I digress.

While some of these changes are not to my taste, there's nothing I can't 
live with for the sake of getting this patch series applied, finally.

> 3. I made it so that delete-dependencies also acts on peerDependencies.
>     That way, we don't have to dance around ordering.

I think this is not quite correct.

(Actually, I suspect more broadly that node-build-system's handling of 
peerDependencies is not quite correct, but wrapping my head around the 
semantics of peerDependencies is on my to-do list for after these 
patches are applied. Here's one thing I want to read and understand: 
https://pnpm.io/how-peers-are-resolved)

NPM does not try to install packages in "peerDependencis" during 'npm 
install' (out 'configure' phase). The problem arises because because our 
'patch-dependencies' phase adds the "peerDependencies" as additional 
"dependencies". (Why? I don't fully understand, but I guess because it 
wants them to be installed.) We want absent "peerDependencies" to not be 
listed in "dependencies", but I don't think we want to delete them from 
"peerDependencies": at a minimum, we do not need to, and it seems like 
it might cause problems that I don't fully understand.

(This is one of the reasons I preferred to handle absent dependencies in 
the 'patch-dependencies' phase.)

> 4. Regexps :)

Hopefully addressed in my previous email :) Jelle makes good arguments 
for the no-regexps side. I'm genuinely on the fence, which suggests to 
me the best course might be to leave it as a possible future extension 
(as we're doing with '#:absent-dependencies').

> PS: If someone else wants to push these in my absence, please adjust the
> signoffs.  Also, if those mail headers are broken, don't forget to reset
> Philip as author.

For the patches where you've made substantive changes to the 
implementation or the commit message, maybe there should be more of an 
annotation than just "Signed-off-by". I'm not super familiar with the 
Git etiquette here, but one suggestion I've seen is something like:

    Signed-off-by: Random J Developer <random@developer.example.org>
    [lucky@maintainer.example.org: struct foo moved from foo.c to foo.h]
    Signed-off-by: Lucky K Maintainer <lucky@maintainer.example.org>

It's not a big deal, I could just imagine getting confused some months 
or years from now about what exactly I wrote or didn't write. (TBH the 
numerous versions of this series are already a bit unwieldy.)

Time permitting, I'll send some more comments, but the only things I 
think need to be addressed before merging are peerDependencies and regexps.

Thanks for helping to keep things moving!

-Philip