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

[Philip McGrath]

Hi!

On 11/14/21 15:44, Liliana Marie Prikler wrote:
> Looking at this patch, it does *a lot* at once and could probably be
> separated into more than one.  Particularly, I'd suggest providing
> capabilities in node-build-system first, then switching over to the new
> thing in node.

Thanks for these comments. Some of the things to which you drew my 
attention seem to have been workarounds for problems that had since been 
solved at a deeper level. Then, in particular, this comment:


>> +(define* (configure-gyp #:key inputs #:allow-other-keys)
>> +  "Run 'node-gyp configure' if we see a 'binding.gyp' file."
>> +  (if (file-exists? "binding.gyp")
>> +      (invoke (which "node-gyp") "configure")
>> +      #t))
>> +
> You might want to make this part of configure itself, though I'm not
> sure what the consensus is there when mixing different build system
> styles.  (invoke (which ...) ) is also a pretty rare pattern, used in
> only four locations so far.

prompted me to look more closely at why so much manual work was needed 
in the first place.

It turns out that the `npm install` in the `'configure` phase should 
have handled most of it automatically, but the Guix packages were 
deleting the configure phase to avoid checking for devDependencies that 
aren't in Guix (or that we just don't want, e.g. some dependencies of 
node-sqlite3).

In v2 of this series (which will follow this email), I've removed all of 
the `node-gyp`-specific build-side code and tried a more general 
solution, adding an `#:absent-packages` argument to instruct the 
`'patch-dependencies` phase to remove the specified packages from the 
"package.json" file. This means that the Guix package can still run 
`'configure`/`npm install`, checking properly for the packages that we 
*do* have/want and doing all of the other automatic work `npm install` 
does. I also like that listing the missing packages in the Guix package 
definition provides a sort of documentation of what is missing: for 
example, it is clear which packages could have their tests enabled with 
the addition of a `node-tap` package, without having to inspect all of 
the individual "package.json" files.

I've updated all of the existing Node.js packages that deleted their 
`'configure` phase to use `#:absent-dependencies` instead.

> > @@ -58,10 +62,13 @@ (define private-keywords
> >                                 `(("source" ,source))
> >                                 '())
> >                           ,@inputs
> > -
> >                           ;; Keep the standard inputs of 'gnu-build-
> > system'.
> >                           ,@(standard-packages)))
> >            (build-inputs `(("node" ,node)
> > +                         ("python" ,python)
> > +                        ;; We don't always need libuv, but the libuv
> > and
> > +                        ;; node versions need to match:
> > +                        ("libuv" ,@(assoc-ref (package-inputs node)
> > "libuv"))
> >                            ,@native-inputs))
> >            (outputs outputs)
> >            (build node-build)
> Will this python input always or often enough be needed?  What's the
> build system ratio on node like, gyp vs. anything else, particularly
> with packages close to the node core?

GYP is a Python program, and it (or at least node's fork of it) expects 
to have a python executable available to invoke with sub-programs. Since 
npm depends on node-gyp and GYP, it is pretty close to the core.

In v2, I've just had packages that use node-gyp add Python to their 
inputs. IIRC, that used to not be enough, but it seems underlying 
problems were fixed in the mean time.

An alternative approach would be to configure it using the npmrc file, 
as I do for `nodedir` in v2. I'm not sure that makes much difference 
either way, but in v2 I've tried to minimize the amount of 
`node-gyp`-specific handling.

-Philip