[bug#51838] [PATCH v3 29/43] gnu: Add node-sqlite3.

[Philip McGrath]

On 12/12/21 10:42, Pierre Langlois wrote:
> Philip McGrath <philip@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