Subject: Re: [PATCH 0/3] guix: node-build-system: Support compiling addons with node-gyp.

From: Pierre Langlois <[email-masked]>
Date: Sat, 02 Oct 2021 12:30:56 +0100

Hi Philip,

Philip McGrath <[email-masked]> writes:

> Hi,
>
> I've reworked the part of the patch series dealing with node-gyp.

Nice, thanks for working on this! I'll take a look at the patches, sorry
I've not had too much time this week yet :-).

> I'd like to find an NPM addon package to submit as part of this series, too,
> basically as a test case. If I can find one that doesn't raise too many other
> complications, I may send this in as a separate patch, but feel free to try it
> with tree-sitter, too.

Cool, I'll give it a test with the tree-sitter series.

>
> There are a few things I'm still not sure about. I haven't made
> node-build-system add libuv as an implicit input, because I think some
> node-gyp addons don't actually need libuv, but maybe it's common enough that
> it should be done automatically.

I see, I assumed libuv was needed for all addons. I'm thinking given
node itself heavily depends on libuv anymays, it might be nice to add it
by default in the build system, but I don't really mind either way. I'll
check if I can just add libuv in the tree-sitter packages.

>
> Likewise, I haven't tried to change the issue of `node` referring to
> `node-bootstrap`, but I still think it should be changed.

I agree it would be good to change this. I started working on updating
node last weekend but haven't had time to work on it after that. I think
we should probably name node always as "node-<version>":

  node -> node-10
  node-lts -> node-14

Then keep the node and node-lts names as aliases:

  (define-public node node-14)     ;; The latest, but move it to node-16 later
  (define-public node-lts node-14)

However, moving the node variable to node-14 is likely to cause
rebuilds, so we'll have to check that.

Anyways, here's my WIP patch in case there's anything of interest
there. Note that it still has the bug you pointed out where I'm refering
to the host python instead of the target one (thanks for pointing it
out!).

Thanks,
Pierre

From b43db52b9a1b55b5756268b710907813f07a42ba Mon Sep 17 00:00:00 2001
From: Pierre Langlois <[email-masked]>
Date: Sun, 26 Sep 2021 18:58:53 +0100
Subject: [PATCH] update node

---
 gnu/packages/adns.scm |  46 ++++++++++-
 gnu/packages/node.scm | 175 +++++++++++++++++++++++++++++++++++++-----
 2 files changed, 198 insertions(+), 23 deletions(-)

diff --git a/gnu/packages/adns.scm b/gnu/packages/adns.scm
index b36ec18462..6fba9783e0 100644
--- a/gnu/packages/adns.scm
+++ b/gnu/packages/adns.scm
@@ -23,8 +23,10 @@
   #:use-module (guix licenses)
   #:use-module (guix packages)
   #:use-module (guix download)
+  #:use-module (guix git-download)
   #:use-module (guix build-system cmake)
   #:use-module (guix build-system gnu)
+  #:use-module (gnu packages autotools)
   #:use-module (gnu packages m4)
   #:use-module (gnu packages pkg-config))

@@ -92,7 +94,7 @@ multiple clients and programs with graphical user interfaces.")
   (package
     (inherit c-ares)
     (name "c-ares")
-    (version "1.17.1")
+    (version "1.17.2")
     (source (origin
               (method url-fetch)
               (uri (string-append
@@ -100,10 +102,46 @@ multiple clients and programs with graphical user interfaces.")
                     ".tar.gz"))
               (sha256
                (base32
-                "0h7wjfnk2092glqcp9mqaax7xx0s13m501z1gi0gsjl2vvvd0gfp"))))
+                "0gcincjvpll2qmlc906jx6mfq97s87mgi0zby0753ki0rr2ch0s8"))))
     (arguments
-     `(;; FIXME: Some tests require network access
-       #:tests? #f))))
+     `(#:phases
+       (modify-phases %standard-phases
+         (replace 'check
+           (lambda* (#:key tests? #:allow-other-keys)
+             (when tests?
+               (invoke "./test/arestest"
+                       (string-append
+                         ;; "Live" tests require network access.
+                         "--gtest_filter=-*.Live*:"
+                         ;; FIXME: This test fails in the build sandbox, but
+                         ;; otherwise passes.
+                         "AddressFamiliesAI/"
+                         "MockChannelTestAI.FamilyV4ServiceName/0"))))))))))
+
+(define-public c-ares-for-node
+  (let ((commit "6299d7be383de62da831a3c48f4017b70c664de8")
+        (revision "1"))
+    (package
+      (inherit c-ares/fixed)
+      (name "c-ares")
+      (version (git-version "1.17.2" revision commit))
+      (source
+       (origin
+         (method git-fetch)
+         (uri (git-reference
+               (url "https://github.com/c-ares/c-ares.git")
+               (commit commit)))
+         (file-name (git-file-name name version))
+         (sha256
+          (base32 "1rmk7m7lv5kmqxhb2dmq5fxk73iicg5rgsz2y855hk0a92xlrcsd"))))
+      (native-inputs
+       `(("pkg-config" ,pkg-config)
+         ("autoconf" ,autoconf)
+         ("automake" ,automake)
+         ("libtool" ,libtool))))))
+      ;; (arguments
+      ;;  `(;; FIXME: Some tests require network access
+      ;;    #:tests? #f)))))

 ;; gRPC requires a c-ares built with CMake in order to get the .cmake modules.
 ;; We can not build c-ares itself with CMake because that would introduce a
diff --git a/gnu/packages/node.scm b/gnu/packages/node.scm
index 71da2aa18f..79ffa8fca9 100644
--- a/gnu/packages/node.scm
+++ b/gnu/packages/node.scm
@@ -53,7 +53,7 @@
   #:use-module (ice-9 match)
   #:use-module (srfi srfi-26))

-(define-public node
+(define-public node-10
   (package
     (name "node")
     (version "10.24.0")
@@ -284,7 +284,7 @@ devices.")
 ;; This should be the latest version of node that still builds without
 ;; depending on llhttp.
 (define-public node-bootstrap
-  (hidden-package node))
+  (hidden-package node-10))

 ;; Duplicate of node-semver
 (define-public node-semver-bootstrap
@@ -524,17 +524,17 @@ Node.js and web browsers.")
 (define-public node-llparse-bootstrap
   (package
     (name "node-llparse")
-    (version "7.1.0")
+    (version "7.1.1")
     (source
      (origin
        (method git-fetch)
        (uri (git-reference
-             (url "https://github.com/indutny/llparse.git")
+             (url "https://github.com/nodejs/llparse.git")
              (commit (string-append "v" version))))
        (file-name (git-file-name name version))
        (sha256
         (base32
-         "10da273iy2if88hp79cwms6c8qpsl1fkgzll6gmqyx5yxv5mkyp6"))
+         "0gzsa4nwrhvm7gz817l5r6v7i8lmqpnrg25smqiq6x8xgs8dlmgl"))
        (modules '((guix build utils)))
        (snippet
         '(begin
@@ -577,7 +577,7 @@ Node.js and web browsers.")
 parser definition into a C output.")
     (license license:expat)))

-(define-public llhttp-bootstrap
+(define-public llhttp-bootstrap-2
   (package
     (name "llhttp")
     (version "2.1.3")
@@ -648,17 +648,42 @@ parser definition into a C output.")
 source files.")
     (license license:expat)))

-(define-public node-lts
+(define-public llhttp-bootstrap
+  (package (inherit llhttp-bootstrap-2)
+    (name "llhttp")
+    (version "6.0.5")
+    (source (origin
+              (method git-fetch)
+              (uri (git-reference
+                    (url "https://github.com/nodejs/llhttp.git")
+                    (commit (string-append "v" version))))
+              (file-name (git-file-name name version))
+              (sha256
+               (base32
+                "1wc5xsa76my32k86cax540q0g4y85w7cikqvdjy3rkz2r7fjlyyk"))
+              (modules '((guix build utils)))
+              (snippet
+               '(begin
+                  ;; Fix imports for esbuild.
+                  ;; https://github.com/evanw/esbuild/issues/477
+                  (substitute* "src/llhttp/http.ts"
+                    (("\\* as assert") "assert"))
+                  (substitute* "Makefile"
+                    (("npx ts-node bin/generate.ts")
+                     "node bin/generate.js"))
+                  #t))))))
+
+(define-public node-14
   (package
-    (inherit node)
-    (version "14.16.0")
+    (inherit node-10)
+    (version "14.17.6")
     (source (origin
               (method url-fetch)
               (uri (string-append "https://nodejs.org/dist/v" version
                                   "/node-v" version ".tar.xz"))
               (sha256
                (base32
-                "19nz2mhmn6ikahxqyna1dn25pb5v3z9vsz9zb2flb6zp2yk4hxjf"))
+                "0pmd0haav2ychhcsw44klx6wfn8c7j1rsw08rc8hcm5i3h5wsn7l"))
               (modules '((guix build utils)))
               (snippet
                `(begin
@@ -675,7 +700,7 @@ source files.")
                     (("deps/zlib/zlib.gyp") ""))
                   #t))))
     (arguments
-     (substitute-keyword-arguments (package-arguments node)
+     (substitute-keyword-arguments (package-arguments node-10)
        ((#:configure-flags configure-flags)
         ''("--shared-cares"
            "--shared-libuv"
@@ -798,6 +823,8 @@ source files.")
                          '("test/parallel/test-dns.js"
                            "test/parallel/test-dns-lookupService-promises.js"))

+               (delete-file "test/parallel/test-https-agent-unref-socket.js")
+
                ;; FIXME: This test fails randomly:
                ;; https://github.com/nodejs/node/issues/31213
                (delete-file "test/parallel/test-net-listen-after-destroying-stdin.js")
@@ -822,9 +849,10 @@ source files.")
                ;; TODO: Regenerate certs instead.
                (for-each delete-file
                          '("test/parallel/test-tls-passphrase.js"
-                           "test/parallel/test-tls-server-verify.js"))
-
-               ;; Replace pre-generated llhttp sources
+                           "test/parallel/test-tls-server-verify.js"))))
+           ;; Replace pre-generated llhttp sources
+           (add-after 'patch-files 'replace-llhttp
+             (lambda* (#:key inputs #:allow-other-keys)
                (let ((llhttp (assoc-ref inputs "llhttp")))
                  (copy-file (string-append llhttp "/src/llhttp.c")
                             "deps/llhttp/src/llhttp.c")
@@ -836,7 +864,7 @@ source files.")
                             "deps/llhttp/include/llhttp.h"))))))))
     (native-inputs
      `(;; Runtime dependencies for binaries used as a bootstrap.
-       ("c-ares" ,c-ares)
+       ("c-ares" ,c-ares-for-node)
        ("google-brotli" ,google-brotli)
        ("icu4c" ,icu4c-67)
        ("libuv" ,libuv-for-node)
@@ -852,20 +880,129 @@ source files.")
     (inputs
      `(("bash" ,bash)
        ("coreutils" ,coreutils)
-       ("c-ares" ,c-ares)
+       ("c-ares" ,c-ares-for-node)
        ("icu4c" ,icu4c-67)
        ("libuv" ,libuv-for-node)
-       ("llhttp" ,llhttp-bootstrap)
+       ("llhttp" ,llhttp-bootstrap-2)
        ("google-brotli" ,google-brotli)
        ("nghttp2" ,nghttp2 "lib")
        ("openssl" ,openssl)
        ("zlib" ,zlib)))))

+(define-public node
+  (package
+    (inherit node-14)
+    (version "16.10.0")
+    (source (origin
+              (method url-fetch)
+              (uri (string-append "https://nodejs.org/dist/v" version
+                                  "/node-v" version ".tar.xz"))
+              (sha256
+               (base32
+                "04krpy0r8msv64rcf0vy2l2yzf0a401km8p5p7h12j9b4g51mp4p"))
+              (modules '((guix build utils)))
+              (snippet
+               `(begin
+                  ;; Remove bundled software, where possible
+                  (for-each delete-file-recursively
+                            '("deps/cares"
+                              "deps/icu-small"
+                              "deps/nghttp2"
+                              "deps/openssl"
+                              "deps/zlib"))
+                  (substitute* "Makefile"
+                    ;; Remove references to bundled software.
+                    (("deps/uv/uv.gyp") "")
+                    (("deps/zlib/zlib.gyp") ""))
+                  #t))))
+    (arguments
+     (substitute-keyword-arguments (package-arguments node-14)
+       ((#:phases phases)
+        `(modify-phases ,phases
+           (replace 'patch-files
+             (lambda* (#:key native-inputs inputs #:allow-other-keys)
+               ;; Fix hardcoded /bin/sh references.
+               (substitute* '("lib/child_process.js"
+                              "lib/internal/v8_prof_polyfill.js"
+                              "test/parallel/test-child-process-spawnsync-shell.js"
+                              "test/parallel/test-fs-write-sigxfsz.js"
+                              "test/parallel/test-stdio-closed.js"
+                              "test/parallel/test-stdin-from-file-spawn.js"
+                              "test/sequential/test-child-process-emfile.js")
+                 (("'/bin/sh'")
+                  (string-append "'" (assoc-ref inputs "bash") "/bin/sh'")))
+
+               ;; Fix hardcoded /usr/bin/env references.
+               (substitute* '("test/parallel/test-child-process-default-options.js"
+                              "test/parallel/test-child-process-env.js"
+                              "test/parallel/test-child-process-exec-env.js")
+                 (("'/usr/bin/env'")
+                  (string-append "'" (assoc-ref inputs "coreutils")
+                                 "/bin/env'")))
+
+               ;; Fix /usr/bin/env shebang in node-gyp.
+               (substitute* "deps/npm/node_modules/node-gyp/bin/node-gyp.js"
+                 (("#!/usr/bin/env")
+                  (string-append "#!" (assoc-ref inputs "coreutils") "/bin/env")))
+
+               (substitute* "deps/npm/node_modules/node-gyp/gyp/gyp_main.py"
+                 (("#!/usr/bin/env python")
+                  (string-append "#!" (assoc-ref (or native-inputs inputs)
+                                                 "python")
+                                 "/bin/python3")))
+
+               ;; FIXME: These tests fail in the build container, but they don't
+               ;; seem to be indicative of real problems in practice.
+               (for-each delete-file
+                         '("test/parallel/test-cluster-primary-error.js"
+                           "test/parallel/test-cluster-primary-kill.js"))
+
+               ;; These require a DNS resolver.
+               (for-each delete-file
+                         '("test/parallel/test-dns.js"
+                           "test/parallel/test-dns-lookupService-promises.js"))
+
+               ;; FIXME: This test fails randomly:
+               ;; https://github.com/nodejs/node/issues/31213
+               (delete-file "test/parallel/test-net-listen-after-destroying-stdin.js")
+
+               ;; FIXME: These tests fail on armhf-linux:
+               ;; https://github.com/nodejs/node/issues/31970
+               ,@(if (target-arm32?)
+                     '((for-each delete-file
+                                 '("test/parallel/test-zlib.js"
+                                   "test/parallel/test-zlib-brotli.js"
+                                   "test/parallel/test-zlib-brotli-flush.js"
+                                   "test/parallel/test-zlib-brotli-from-brotli.js"
+                                   "test/parallel/test-zlib-brotli-from-string.js"
+                                   "test/parallel/test-zlib-convenience-methods.js"
+                                   "test/parallel/test-zlib-random-byte-pipes.js"
+                                   "test/parallel/test-zlib-write-after-flush.js")))
+                     '())
+
+               ;; These tests have an expiry date: they depend on the validity of
+               ;; TLS certificates that are bundled with the source.  We want this
+               ;; package to be reproducible forever, so remove those.
+               ;; TODO: Regenerate certs instead.
+               (for-each delete-file
+                         '("test/parallel/test-tls-passphrase.js"
+                           "test/parallel/test-tls-server-verify.js"))))))))
+    (native-inputs
+     (alist-replace "icu4c" (list icu4c-68)
+                    (package-native-inputs node-14)))
+    (inputs
+     (alist-replace "icu4c" (list icu4c-68)
+                    (alist-replace "llhttp" (list llhttp-bootstrap)
+                                   (package-inputs node-14))))))
+
+;; LTS release used by the node build system.
+(define-public node-lts node-14)
+
 (define-public libnode
-  (package/inherit node
+  (package/inherit node-10
     (name "libnode")
     (arguments
-     (substitute-keyword-arguments (package-arguments node)
+     (substitute-keyword-arguments (package-arguments node-10)
        ((#:configure-flags flags ''())
         `(cons* "--shared" "--without-npm" ,flags))
        ((#:phases phases '%standard-phases)
--
2.33.0