Package: guix-patches;
Reported by: Pierre Langlois <pierre.langlois <at> gmx.com>
Date: Sun, 8 Aug 2021 23:27:01 UTC
Severity: normal
Tags: patch
Message #224 received at 49946 <at> debbugs.gnu.org (full text, mbox):
From: Philip McGrath <philip <at> philipmcgrath.com> To: Pierre Langlois <pierre.langlois <at> gmx.com> Cc: 49946 <at> debbugs.gnu.org, Maxime Devos <maximedevos <at> telenet.be> Subject: Re: [bug#49946] [PATCH 08/31] gnu: node: Patch /usr/bin/env in node-gyp. Date: Sun, 26 Sep 2021 18:02:47 -0400
Hi Pierre,
On 9/25/21 6:24 AM, Pierre Langlois wrote:
> Philip McGrath <philip <at> 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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.