Subject: Re: [bug#49946] [PATCH 08/31] gnu: node: Patch /usr/bin/env in node-gyp.

From: Philip McGrath <[email-masked]>
Date: Sun, 26 Sep 2021 18:02:47 -0400

Hi Pierre,

On 9/25/21 6:24 AM, Pierre Langlois wrote:
> Philip McGrath <[email-masked]> writes:
>> 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
I think it might be better to use `patch-shebang` from `(guix build 
utils)` rather than `substitute*` these by hand, and it seems that 
`patch-shebang` removed the indirection through `env`. My guess is most 
of these cases are to accommodate the fact that `node` and `python` are 
often installed to places other than `/usr/bin`.

 >> 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?

Looking at `gnu-build-system`, it seems that the `'patch-shebangs` phase 
only operates on files installed in the "/bin" and "/sbin" 
subdirectories of the package's outputs. That restriction doesn't make 
sense to me in general: for instance, what about "/libexec"? For Node 
specifically, this misses a lot of stuff under "/lib/node_modules" and 
"/lib/node_modules/npm/node_modules". I think I more general fix could 
subsume the `'patch-npm-shebang` and `'patch-node-shebang` phases in 
building Node, too.

 > 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

I think in some places (but perhaps not enough places) Guix uses 
`python-wrapper` to work around this ...

 > Maybe updating node would be better than this fix though.

I'm not totally clear on whether the upstream fix is in 14.17.6 LTS, 
but, if so, that seems great!

> 
>> 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.

Since these are developed and released with Node, and apparently we can 
build them as part of the Node build process, I was thinking we could 
just make packages that point to these versions we're already building. 
It might be good to hear from someone who develops with node/npm, though 
... I just use it to install software that I can't find packaged elsewhere.

> 
>> 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"))

I found it worked to just add libuv as an input of packages built with 
node-gyp. I hadn't tried to change `node-build-system`, but I think that 
would be the place to do it.

> 
>    - 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)

That seems right. I believe there's a similar "npm_config_python" for 
the Python executable to use.

Alternatively, I think it's possible to configure these in 
$PREFIX/etc/npmrc: <https://docs.npmjs.com/cli/v7/configuring-npm/npmrc>

> 
> 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.

Glad it was useful!

For patching the shebangs, here's a variant of node-lts that worked for 
me, though I think it would be even better to combine it with the 
existing phases:

```
(define-public patched-node
   (let ((node node-lts))
     (package
       (inherit node)
       (arguments
        (substitute-keyword-arguments (package-arguments node)
          ((#:phases standard-phases)
           `(modify-phases ,standard-phases
              (add-after 'patch-npm-shebang 'patch-more-shebangs
                (lambda* (#:key inputs outputs #:allow-other-keys)
                  (define (append-map f lst)
                    (apply append (map f lst)))
                  ;; from patch-shebangs
                  (define bin-directories
                    ;;(match-lambda
                    ;;  ((_ . dir)
                    (lambda (pr)
                      (let ((dir (cdr pr)))
                        (list (string-append dir "/bin")
                              (string-append dir "/sbin")))))
                  (define output-bindirs
                    (append-map bin-directories outputs))
                  (define input-bindirs
                    ;; Shebangs should refer to binaries of the target 
system---i.e., from
                    ;; "inputs", not from "native-inputs".
                    (append-map bin-directories inputs))
                  (define path
                    (append output-bindirs input-bindirs))
                  (with-directory-excursion
                      (string-append (assoc-ref outputs "out")
                                     "/lib/node_modules/npm/node_modules")
                    (for-each
                     ;;(cut patch-shebang <> path)
                     (lambda (file)
                       (patch-shebang file path))
                     ;; from patch-generated-file-shebangs
                     (find-files "."
                                 (lambda (file stat)
                                   (and (eq? 'regular (stat:type stat))
                                        (not (zero? (logand (stat:mode 
stat) #o100)))))
                                 #:stat lstat))))))))))))
```

-Philip