Package: guix-patches;
Reported by: Philip McGrath <philip <at> philipmcgrath.com>
Date: Sun, 14 Nov 2021 12:43:01 UTC
Severity: normal
Tags: patch
Done: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Philip McGrath <philip <at> philipmcgrath.com> To: Pierre Langlois <pierre.langlois <at> gmx.com> Cc: 51838 <at> debbugs.gnu.org Subject: [bug#51838] [PATCH v3 29/43] gnu: Add node-sqlite3. Date: Sun, 12 Dec 2021 16:18:36 -0500
On 12/12/21 10:42, Pierre Langlois wrote:
>
> Philip McGrath <philip <at> philipmcgrath.com> writes:
>
>> * gnu/packages/node-xyz.scm (node-sqlite3): New variable.
>> ---
>> gnu/packages/node-xyz.scm | 118 +++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 115 insertions(+), 3 deletions(-)
>>
>> diff --git a/gnu/packages/node-xyz.scm b/gnu/packages/node-xyz.scm
>> index 60dbfc163c..b979d0cd53 100644
>> --- a/gnu/packages/node-xyz.scm
>> +++ b/gnu/packages/node-xyz.scm
>> @@ -615,7 +615,8 @@ (define-public node-addon-api
>> (sha256
>> (base32 "1bhvfi2m9nxfz418s619914vmidcnrzbjv6l9nid476c3zlpazch"))))
>> (inputs
>> - `(("python" ,python)))
>> + `(("python" ,python)
>> + ("node-safe-buffer" ,node-safe-buffer)))
>> (build-system node-build-system)
>> (arguments
>> `(#:absent-dependencies
>> @@ -630,8 +631,7 @@ (define-public node-addon-api
>> "eslint-plugin-promise"
>> "fs-extra"
>> "path"
>> - "pre-commit"
>> - "safe-buffer")
>> + "pre-commit")
>> #:phases
>> (modify-phases %standard-phases
>> (add-after 'unpack 'skip-js-tests
>
> nit: Did you mean to include these changes in this patch? It seems they
> should be part of the node-addon-api patch.
Thanks! I've rebased, reorganized, and squashed this series a number of
times: these must have ended up in the wrong place.
>> + "aws-sdk"
>> + "@mapbox/cloudfriend"
>> + ;; Confusingly, this is only a dependency beceuse of
>
> typo: beceuse -> because
Thanks, will fix!
>> + (add-after 'patch-dependencies 'avoid-node-pre-gyp
>> + (lambda args
>> + (substitute* ".npmignore"
>> + (("lib/binding")
>> + "#lib/binding # <- patched for Guix"))
>
> Would this substitute* be more suited to live in the 'patch-binding-path
> phase?
No, at least not as currently written.
The upstream .npmignore file excludes the compiled addon from `npm pack`
and `npm install`, so patching it after Guix's install phase would be
too late. (Except that, unfortunately, Guix doesn't seem to be
respecting instructions about which files to include or not in built
packages ...)
The 'patch-binding-path phase, on the other hand, replaces code that
dynamically finds the addon to load at runtime via the
`@mapbox/node-pre-gyp` package (which we don't have) with one that
simply uses the path we built directly. Because that path depends (or
should---see below) on the expansion of the `module_path` configuration,
including parameters like `napi_build_version`, the most reliable
approach seems to be patching this after Guix's configure phase, so we
can just find what path we actually did build, rather than having to
accurately predict what we're going to build.
Instead of all of this, Debian packages a patched version of
`@mapbox/node-pre-gyp` that always builds from source. This has some
appeal and might mean less patching overall. I started down that road in
<https://gitlab.com/philip1/guix-patches/-/tree/wip-node-npm-gyp-hist-4-unicode-bootstrap>,
but I found `@mapbox/node-pre-gyp` really did need its `npm-log`
dependency: we have `npm-log` as a dependency of `npm`, but, when I
tried to package it properly, I found (if I'm remembering the details
correctly) that bootstrapping `@unicode/14.0.0` from
https://github.com/node-unicode/node-unicode-data seemed to involve a
dependency cycle. That seemed like a headache for another time.
>> + (with-atomic-file-replacement "package.json"
>> + (lambda (in out)
>> + (let* ((js (read-json in))
>> + (alist (match js
>> + (('@ . alist) alist)))
>> + (scripts-alist (match (assoc-ref alist "scripts")
>> + (('@ . alist) alist)))
>> + (scripts-alist
>> + ;; install script would use node-pre-gyp
>> + (assoc-remove! scripts-alist "install"))
>> + (alist
>> + (assoc-set! alist "scripts" (cons '@ scripts-alist)))
>> + (binary-alist (match (assoc-ref alist "binary")
>> + (('@ . alist) alist)))
>> + (js (cons '@ alist)))
>> + ;; compensate for lack of @mapbox/node-pre-gyp
>> + (setenv "GYP_DEFINES"
>> + (string-append
>> + "module_name="
>> + (assoc-ref binary-alist "module_name")
>> + " "
>> + "module_path="
>> + (assoc-ref binary-alist "module_path")))
>> + (write-json js
>> + out)))))))))
>
> I was having a bit of a hard time understand this phase, let me know if
> I have this right. We have this JSON input:
>
> --8<---------------cut here---------------start------------->8---
> "binary": {
> "module_name": "node_sqlite3",
> "module_path": "./lib/binding/napi-v{napi_build_version}-{platform}-{arch}",
> "host": "https://mapbox-node-binary.s3.amazonaws.com",
> "remote_path": "./{name}/v{version}/{toolset}/",
> "package_name": "napi-v{napi_build_version}-{platform}-{arch}.tar.gz",
> "napi_versions": [
> 3
> ]
> },
>
> "scripts": {
> "install": "node-pre-gyp install --fallback-to-build",
> "pretest": "node test/support/createdb.js",
> "test": "mocha -R spec --timeout 480000",
> "pack": "node-pre-gyp package"
> },
> --8<---------------cut here---------------end--------------->8---
>
> And we:
>
> - Read the "binary" entry to find the module_name and module_path to
> give to node-gyp, so we can use our own GYP instead of a bundled one.
>
> - Delete the "scripts.install" phase, it's not using the correct GYP.
>
> Maybe a couple of comments would be helpful here :-).
Yes, I'll add comments :)
What you said is mostly right, but, to clarify, the upstream
scripts.install is using node-PRE-gyp, which tries to download pre-built
binaries from S3-compatible storage. Since we don't have a patched
version like Debian, we definitely want to avoid that.
When node-pre-gyp does decide to build from source, it arranges to
supply module_name and module_path based on the package.json
definitions, so the binding.gyp doesn't define them. Thus, we also have
to pass them to node-gyp, and the GYP_DEFINES environment variable turns
out to be the easiest way to make sure they get passed on from npm to
node-gyp to gyp.
What we do isn't quite consistent with node-pre-gyp because we don't
currently perform substitution on the module_path, so there ends up
being a directory literally named
"napi-v{napi_build_version}-{platform}-{arch}". But that could be
improved later.
-Philip
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.