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

* gnu/packages/node-xyz.scm (node-sqlite3): New variable.
---
 gnu/packages/node-xyz.scm | 118 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 115 insertions(+), 3 deletions(-)

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.

> @@ -660,3 +660,115 @@ (define-public node-addon-api
>  @code{libuv} (included in a project via @code{#include <uv.h>}) are not
>  ABI-stable across Node.js major versions.")
>      (license license:expat)))
> +
> +(define-public node-sqlite3
> +  (package
> +    (name "node-sqlite3")
> +    (version "5.0.2")
> +    (source
> +     (origin
> +       (method git-fetch)
> +       (uri (git-reference
> +             (url "https://github.com/mapbox/node-sqlite3")
> +             (commit (string-append "v" version))))
> +       (file-name (git-file-name name version))
> +       (sha256
> +        (base32 "0sbbzzli282nxyfha10zx0k5m8hdp0sf3ipl59khjb7wm449j86h"))
> +       (snippet
> +        (with-imported-modules '((guix build utils))
> +          #~(begin
> +              (use-modules (guix build utils))
> +              ;; unbundle sqlite
> +              (for-each delete-file-recursively
> +                        (find-files "deps"
> +                                    (lambda (pth stat)
> +                                      (gzip-file? pth)))))))))
> +    (inputs
> +     `(("node-addon-api" ,node-addon-api)
> +       ("python" ,python)
> +       ("sqlite" ,sqlite)))
> +    (build-system node-build-system)
> +    (arguments
> +     `(#:tests?
> +       #f ; FIXME: tests depend on node-mocha
> +       #:modules
> +       ((guix build node-build-system)
> +        (guix build json)
> +        (srfi srfi-1)
> +        (ice-9 match)
> +        (guix build utils))
> +       #:absent-dependencies
> +       `(;; Normally, this is "built" using @mapbox/node-pre-gyp,
> +         ;; which publishes or downloads pre-built binaries
> +         ;; or falls back to building from source.
> +         ;; Here, we patch out all of that and just build directly.
> +         ;; It would be better to patch a version of @mapbox/node-pre-gyp
> +         ;; that always builds from source, as Debian does,
> +         ;; but there are a number of dependencies that need
> +         ;; to be packaged or removed.
> +         "@mapbox/node-pre-gyp"
> +         "node-pre-gyp" ;; deprecated name still used in some places
> +         "aws-sdk"
> +         "@mapbox/cloudfriend"
> +         ;; Confusingly, this is only a dependency beceuse of

typo: beceuse -> because

> +         ;; @mapbox/node-pre-gyp: with that removed,
> +         ;; npm will use its own copy:
> +         "node-gyp"
> +         ;; These we'd like, we just don't have them yet:
> +         "eslint"
> +         "mocha")
> +       #:phases
> +       (modify-phases %standard-phases
> +         (add-before 'configure 'npm-config-sqlite
> +           ;; We need this step even if we do replace @mapbox/node-pre-gyp
> +           ;; because the package expects to build its bundled sqlite
> +           (lambda* (#:key inputs #:allow-other-keys)
> +             (setenv "npm_config_sqlite" (assoc-ref inputs "sqlite"))))
> +         (add-after 'install 'patch-binding-path
> +           (lambda* (#:key outputs #:allow-other-keys)
> +             (with-directory-excursion
> +                 (string-append (assoc-ref outputs "out")
> +                                "/lib/node_modules/sqlite3/lib")
> +               (match (find-files "binding" "\\.node$")
> +                 ((rel-path)
> +                  (with-atomic-file-replacement "sqlite3-binding.js"
> +                    (lambda (in out)
> +                      (format out "var binding = require('./~a');\n" rel-path)
> +                      (display "module.exports = exports = binding;\n" out))))))))
> +         (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?

> +             (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 :-).

> +    (home-page "https://github.com/mapbox/node-sqlite3")
> +    (synopsis "Asynchronous, non-blocking SQLite3 bindings for Node.js")
> +    (description
> +     "The Node.js add-on @code{node-sqlite3} provides a set of a asynchronous,
> +non-blocking bindings for SQLite3, written in modern C++ and tested for memory
> +leaks.")
> +     (license license:bsd-3)))

Otherwise this looks good to me!

Thanks,
Pierre

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