diff mbox series

[bug#49946,1/3] gnu: node: Avoid duplicating build phases.

Message ID 20210930225611.2143194-2-philip@philipmcgrath.com
State New
Headers show
Series guix: node-build-system: Support compiling addons with node-gyp. | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job
cbaines/issue success View issue
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job
cbaines/issue success View issue
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job
cbaines/issue success View issue
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job
cbaines/issue success View issue
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job
cbaines/issue success View issue

Commit Message

Philip McGrath Sept. 30, 2021, 10:56 p.m. UTC
* gnu/packages/node.scm (node)[arguments]: Split 'patch-files phase
into 'patch-hardcoded-program-references and
'delete-problematic-tests. Adapt those phases and 'configure to work
unmodified on node-lts.
(node, node-lts)[inputs]: Use bash-minimal rather than bash.
(node-lts)[arguments]: Inherit 'patch-hardcoded-program-references,
'delete-problemating-tests, and 'configure phases from the bootstrap
node. Remove the 'patch-files phase, keeping its remaining
non-inherited work in a new 'replace-llhttp-sources phase.
---
 gnu/packages/node.scm | 150 +++++++++++-------------------------------
 1 file changed, 39 insertions(+), 111 deletions(-)

Comments

Pierre Langlois Oct. 2, 2021, 11:03 a.m. UTC | #1
Hi Philip,

Philip McGrath <philip@philipmcgrath.com> writes:

> * gnu/packages/node.scm (node)[arguments]: Split 'patch-files phase
> into 'patch-hardcoded-program-references and
> 'delete-problematic-tests. Adapt those phases and 'configure to work
> unmodified on node-lts.
> (node, node-lts)[inputs]: Use bash-minimal rather than bash.
> (node-lts)[arguments]: Inherit 'patch-hardcoded-program-references,
> 'delete-problemating-tests, and 'configure phases from the bootstrap
> node. Remove the 'patch-files phase, keeping its remaining
> non-inherited work in a new 'replace-llhttp-sources phase.

While I agree that most of the time, factoring out common code is a good
thing, I'm not sure it applies in the case of patching tests. The list
of tests is specific to a version and it's likely for each version to
need fixes. Having a common phase that describes the tests to patch for
2 versions (3 if we add node 16) is harder to maintain than three phases
IMO, even though they'll look similar indeed. Having to change commmon
code can also cause unnessecary rebuilds.

For example, I started working updating node last weekend and saw these
test changes:

 - 14.16 -> 14.17: Delete test/parallel/test-https-agent-unref-socket.js,
                   requires networking
 - 16: Extra test needs /bin/sh patched test/parallel/test-stdin-from-file-spawn.js"
       A couple tests were renamed:
       test/parallel/test-cluster-master-error.js -> test/parallel/test-cluster-primary-error.js
       test/parallel/test-cluster-master-kill.js -> test/parallel/test-cluster-primary-kill.js

That being said, I definetely agree we should have a separate phase for
the replacement of the llhttp source, that's logically different from
patching tests, and is unlikely to change version to version.

Keeping the list of tests local to each packages allows to add node 16
while avoiding rebuilding the others, does this make sense? I could be
wrong here of course :-).

Thanks,
Pierre
diff mbox series

Patch

diff --git a/gnu/packages/node.scm b/gnu/packages/node.scm
index f8ac95884c..34c2bfa9d4 100644
--- a/gnu/packages/node.scm
+++ b/gnu/packages/node.scm
@@ -9,6 +9,7 @@ 
 ;;; Copyright © 2020, 2021 Pierre Langlois <pierre.langlois@gmx.com>
 ;;; Copyright © 2020 Ricardo Wurmus <rekado@elephly.net>
 ;;; Copyright © 2021 Simon Tournier <zimon.toutoune@gmail.com>
+;;; Copyright © 2021 Philip McGrath <philip@philipmcgrath.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -105,14 +106,22 @@ 
        #:test-target "test-ci-js"
        #:phases
        (modify-phases %standard-phases
-         (add-before 'configure 'patch-files
+         (add-before 'configure 'patch-hardcoded-program-references
            (lambda* (#:key 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-stdio-closed.js"
-                            "test/sequential/test-child-process-emfile.js")
+             (substitute*
+                 (let ((common
+                        '("lib/child_process.js"
+                          "lib/internal/v8_prof_polyfill.js"
+                          "test/parallel/test-child-process-spawnsync-shell.js"
+                          "test/parallel/test-stdio-closed.js"
+                          "test/sequential/test-child-process-emfile.js"))
+                       ;; not in bootstap node:
+                       (sigxfsz "test/parallel/test-fs-write-sigxfsz.js"))
+                   (if (file-exists? sigxfsz)
+                       (cons sigxfsz common)
+                       common))
                (("'/bin/sh'")
                 (string-append "'" (assoc-ref inputs "bash") "/bin/sh'")))
 
@@ -122,18 +131,28 @@ 
                             "test/parallel/test-child-process-exec-env.js")
                (("'/usr/bin/env'")
                 (string-append "'" (assoc-ref inputs "coreutils")
-                               "/bin/env'")))
+                               "/bin/env'")))))
+         (add-after 'patch-hardcoded-program-references 'delete-problematic-tests
+           (lambda* (#:key inputs #:allow-other-keys)
+             (define (delete-file-if-exists pth)
+               (when (file-exists? pth)
+                 (delete-file pth)))
 
              ;; 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
+             (for-each delete-file-if-exists
                        '("test/parallel/test-cluster-master-error.js"
                          "test/parallel/test-cluster-master-kill.js"
                          ;; See also <https://github.com/nodejs/node/issues/25903>.
+                         ;; (no longer exists in node-lts)
                          "test/sequential/test-performance.js"))
 
-             ;; This requires a DNS resolver.
-             (delete-file "test/parallel/test-dns.js")
+             ;; These require a DNS resolver.
+             (for-each delete-file-if-exists
+                       '("test/parallel/test-dns.js"
+                         ;; not in the bootstrap node:
+                         "test/parallel/test-dns-lookupService-promises.js"))
+
 
              ;; FIXME: This test fails randomly:
              ;; https://github.com/nodejs/node/issues/31213
@@ -217,9 +236,13 @@ 
                (setenv "CXX" ,(cxx-for-target))
                (setenv "PKG_CONFIG" ,(pkg-config-for-target))
                (apply invoke
-                      (string-append (assoc-ref (or native-inputs inputs)
-                                                "python")
-                                     "/bin/python")
+                      (let ((python
+                             (string-append (assoc-ref (or native-inputs inputs)
+                                                       "python")
+                                            "/bin/python")))
+                        (if (file-exists? python)
+                            python
+                            (string-append python "3")))
                       "configure" flags))))
          (add-after 'patch-shebangs 'patch-npm-shebang
            (lambda* (#:key outputs #:allow-other-keys)
@@ -256,7 +279,7 @@ 
             (variable "NODE_PATH")
             (files '("lib/node_modules")))))
     (inputs
-     `(("bash" ,bash)
+     `(("bash" ,bash-minimal)
        ("coreutils" ,coreutils)
        ("c-ares" ,c-ares)
        ("http-parser" ,http-parser)
@@ -711,103 +734,8 @@  source files.")
                                    libuv "/lib:"
                                    zlib "/lib"
                                    "'],"))))))
-           (replace 'configure
-             ;; Node's configure script is actually a python script, so we can't
-             ;; run it with bash.
-             (lambda* (#:key outputs (configure-flags '()) native-inputs inputs
-                       #:allow-other-keys)
-               (let* ((prefix (assoc-ref outputs "out"))
-                      (xflags ,(if (%current-target-system)
-                                   `'("--cross-compiling"
-                                     ,(string-append
-                                       "--dest-cpu="
-                                       (match (%current-target-system)
-                                         ((? (cut string-prefix? "arm" <>))
-                                          "arm")
-                                         ((? (cut string-prefix? "aarch64" <>))
-                                          "arm64")
-                                         ((? (cut string-prefix? "i686" <>))
-                                          "ia32")
-                                         ((? (cut string-prefix? "x86_64" <>))
-                                          "x64")
-                                         ((? (cut string-prefix? "powerpc64" <>))
-                                          "ppc64")
-                                         (_ "unsupported"))))
-                                   ''()))
-                      (flags (cons
-                               (string-append "--prefix=" prefix)
-                               (append xflags configure-flags))))
-                 (format #t "build directory: ~s~%" (getcwd))
-                 (format #t "configure flags: ~s~%" flags)
-                 ;; Node's configure script expects the CC environment variable to
-                 ;; be set.
-                 (setenv "CC_host" "gcc")
-                 (setenv "CXX_host" "g++")
-                 (setenv "CC" ,(cc-for-target))
-                 (setenv "CXX" ,(cxx-for-target))
-                 (setenv "PKG_CONFIG" ,(pkg-config-for-target))
-                 (apply invoke
-                        (string-append (assoc-ref (or native-inputs inputs)
-                                                  "python")
-                                       "/bin/python3")
-                        "configure" flags))))
-           (replace 'patch-files
+           (add-after 'delete-problematic-tests 'replace-llhttp-sources
              (lambda* (#:key 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/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'")))
-
-               ;; 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-master-error.js"
-                           "test/parallel/test-cluster-master-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"))
-
                ;; Replace pre-generated llhttp sources
                (let ((llhttp (assoc-ref inputs "llhttp")))
                  (copy-file (string-append llhttp "/src/llhttp.c")
@@ -834,7 +762,7 @@  source files.")
        ("python" ,python)
        ("util-linux" ,util-linux)))
     (inputs
-     `(("bash" ,bash)
+     `(("bash" ,bash-minimal)
        ("coreutils" ,coreutils)
        ("c-ares" ,c-ares)
        ("icu4c" ,icu4c-67)