Hi Philip,

Philip McGrath <philip@philipmcgrath.com> writes:

> I'm interested in the node-gyp part of this, which has come up in some other
> software I'm trying to package. These comments come with the caveat that my
> experience with node.js and npm is fairly shallow.

Thanks for your feedback!

>
> On 8/10/21 2:28 PM, Maxime Devos wrote:
>> Pierre Langlois schreef op ma 09-08-2021 om 00:33 [+0100]:
>>> @@ -120,6 +120,10 @@
>>>                  (("'/usr/bin/env'")
>>>                   (string-append "'" (which "env") "'")))
>>>
>>> +             ;; Fix /usr/bin/env shebang in node-gyp.
>>> +             (substitute* "deps/npm/node_modules/node-gyp/bin/node-gyp.js"
>>> +               (("#!/usr/bin/env") (string-append "#!" (which "env"))))
>> For cross-compilation, this should most likely be
>> (string-append (assoc-ref inputs "coreutils") "/bin/env")
>> or something like that instead.  Likewise in other places.
>
> Since the shebang line for node-gyp is specifically "#!/usr/bin/env node", I
> wonder if it should use the node built by this package, rather than a dynamic
> node.

Yeah we could do that, although I generally prefer to follow whatever
the script already does, there could be a good reason for them to use
`env' no?

> More generally, I see that there are 355 directories installed under
> "lib/node_modules/npm/node_modules" (which corresponds to the "deps" 
> path above). Most of them don't seem to be available as Guix packages that could
> be depended upon by other Guix node packages.

Yeah that's tricky, ideally we should remove all the node_modules deps
and package them separately, I wonder if anybody tried to do that
already. I would suspect it to be quite a lot of work, sometimes
unbundling stops being worth and when it's hard to maintain dependencies
manually.

Hopefully we can get there one day though! I don't want to deter anybody
from trying :-), I might give it a go on a raindy day.

> I'd guess node-gyp may not be the only one with shebangs that ought to
> be patched.

Yeah there could be others, although normally the patching phase from
the gnu build system should have taken care of most of them, hopefully
all, I'm not sure why it didn't work for /usr/bin/env though.

I would suggest we patch things as we encounter them, did you find
anymore issues when working on your package?

For instance, while working on a newer version of one of the packages in
this series, I saw we may need to patch GYP's python reference as well,
like so:

(substitute* "deps/npm/node_modules/node-gyp/gyp/gyp_main.py"
  (("#!/usr/bin/env python")
   (string-append "#!" (assoc-ref inputs "python") "/bin/python3")))

Only for node 14+. The reason seems to be that gyp still refers to
"python", but python2 is no longer a dependency for newer nodes. And it
seems GYP is perfectly happy with python3, and the shebang is fixed
upstream so a never node will be fine:
https://github.com/nodejs/node-gyp/pull/2355/files

Maybe updating node would be better than this fix though.

> On 8/8/21 6:29 PM, Pierre Langlois wrote:
>
>>    ... `node-gyp' needs
>
>>    node headers to compile against, packaged as a tarball, which it tries
>
>>    to download.  Instead, we can run a `node-gyp --tarball <> configure'
>
>>    step to manually provide the tarball, which we can package separately
>
>>    for any given node version.
>
> There is also a --nodedir option, which I found could work with something like:
>
>     (string-append "--nodedir=" (assoc-ref inputs "node"))
>
> That seems like it might be better, though I don't know all the considerations
> for cross-compilation and such.

Oh that's a good idea, I didn't really like having to download the
headers separately from the main package, especially given we run
snippet on the source to remove bundled dependencies.

Trying this out this approach does work, but I needed to:

  - Create a union directory with both node and libuv. The node package
    only has headers for V8/node, but we also need libuv, so doing
    something like this works:

    (union-build node-sources
                (list (assoc-ref inputs "node")
                      (assoc-ref inputs "libuv"))
                #:create-all-directories? #t
                #:log-port (%make-void-port "w"))

  - For some reason, --nodedir didn't really "configure" gyp to use that
    node directory, I think it's meant to be passed everytime you run
    any gyp command. Instead I found that you can use and environment
    variable:

    (setenv "npm_config_nodedir" node-sources)

And that works for the packages in this series!  That'll be much better
than before, I'll do it this way.

Thanks again for taking a look, I'll see if I can send updated patches
sometimes this weekend.

Pierre