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

[Philip McGrath]

Hi Pierre,

On 9/25/21 6:24 AM, Pierre Langlois wrote:
> Philip McGrath <philip@philipmcgrath.com> 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