bug#43228: r-v8 doesn't build since node 10.22 update

bug#43228: r-v8 doesn't build since node 10.22 update

[Pierre Langlois]

[-- Attachment #1: Type: text/plain, Size: 795 bytes --]

Hello Guix!

I'm afraid I broke r-v8 and a few of its dependants by updating node,
sorry about that!

AFAIK, the new node uses a function from nghttp2 1.41 that's not present
in 1.40, `nghttp2_option_set_max_settings'. However, since curl depends
on nghttp2 we've grafted 1.40 -> 1.41 to avoid a full rebuild.

Looking at r-v8's log [0], it complains that the symbol is missing,
indicating it's trying to link with the old version 1.40. I /believe/
it's inherited it through r-curl.

I'm not sure how to fix this, I'm happy to revert the node update if
needed, let me know! Then we'd have to wait for the next core-updates
cycle so that we no longer graft nghttp2.

Unless somebody has a better idea?

Thanks,
Pierre

[0]: https://ci.guix.gnu.org/log/62nkhf9dnlzgw3dz9khd79khqdpaib79-r-v8-3.2.0

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 519 bytes --]

bug#43228: r-v8 doesn't build since node 10.22 update

[Mark H Weaver]

Hi Pierre,

I'm quoting your message out of order to ease my reply.

Pierre Langlois <pierre.langlois@gmx.com> writes:

> I'm afraid I broke r-v8 and a few of its dependants by updating node,
> sorry about that!
[...]
> I'm not sure how to fix this, I'm happy to revert the node update if
> needed, let me know! Then we'd have to wait for the next core-updates
> cycle so that we no longer graft nghttp2.

We will actually need node-10.22 (or at least 10.21) in 'master' in just
over 2 weeks, when we'll be compelled to update IceCat to version 78.
On Sept 22, Mozilla is scheduled to release a new batch of security
fixes in 78.3, and there will be no corresponding 68.x release.

(In fact, I had an *identical* commit on my private branch to update
'node' to 10.22, to allow testing IceCat 78 WIP.)

However, if needed, I suppose it might be sufficient for my purposes to
leave 'node' at 10.19.0, and to bind a separate 'node-10.22' variable to
the new version.

> AFAIK, the new node uses a function from nghttp2 1.41 that's not
> present in 1.40, `nghttp2_option_set_max_settings'. However, since curl
> depends on nghttp2 we've grafted 1.40 -> 1.41 to avoid a full rebuild.
>
> Looking at r-v8's log [0], it complains that the symbol is missing,
> indicating it's trying to link with the old version 1.40. I /believe/
> it's inherited it through r-curl.

If grafting is working as it should, then nghttp2-1.40 should never be
linked at runtime.  However, it is certainly the case that most things
(except node-10.22) are *built* against nghttp2-1.40, where the
aforementioned symbol is missing.

One possible solution might be to update the replacement (graft) for
_curl_ so that it's *built* against nghttp2-1.41.  Something like this
(untested):

--8<---------------cut here---------------start------------->8---
diff --git a/gnu/packages/curl.scm b/gnu/packages/curl.scm
index 55b7e4393b..bfcb52b678 100644
--- a/gnu/packages/curl.scm
+++ b/gnu/packages/curl.scm
@@ -183,6 +183,9 @@ tunneling, and so on.")
               (sha256
                (base32
                 "0wlppmx9iry8slh4pqcxj7lwc6fqwnlhh9ri2pcym2rx76a8gwfd"))))
+    (inputs
+     `(("nghttp2" ,nghttp2-1.41 "lib")
+       ,@(alist-delete "nghttp2" (package-inputs curl))))
     (arguments
      (substitute-keyword-arguments (package-arguments curl)
        ((#:phases phases)
--8<---------------cut here---------------end--------------->8---

Would you like to try this and see if it solves the problem?

       Mark

bug#43228: r-v8 doesn't build since node 10.22 update

[Pierre Langlois]

[-- Attachment #1: Type: text/plain, Size: 4880 bytes --]

Hi Mark,

Mark H Weaver writes:

> Hi Pierre,
>
> I'm quoting your message out of order to ease my reply.
>
> Pierre Langlois <pierre.langlois@gmx.com> writes:
>
>> I'm afraid I broke r-v8 and a few of its dependants by updating node,
>> sorry about that!
> [...]
>> I'm not sure how to fix this, I'm happy to revert the node update if
>> needed, let me know! Then we'd have to wait for the next core-updates
>> cycle so that we no longer graft nghttp2.
>
> We will actually need node-10.22 (or at least 10.21) in 'master' in just
> over 2 weeks, when we'll be compelled to update IceCat to version 78.
> On Sept 22, Mozilla is scheduled to release a new batch of security
> fixes in 78.3, and there will be no corresponding 68.x release.

Oooh cool! Looking forwards to icecat 78!

> (In fact, I had an *identical* commit on my private branch to update
> 'node' to 10.22, to allow testing IceCat 78 WIP.)
>
> However, if needed, I suppose it might be sufficient for my purposes to
> leave 'node' at 10.19.0, and to bind a separate 'node-10.22' variable to
> the new version.
>
>> AFAIK, the new node uses a function from nghttp2 1.41 that's not
>> present in 1.40, `nghttp2_option_set_max_settings'. However, since curl
>> depends on nghttp2 we've grafted 1.40 -> 1.41 to avoid a full rebuild.
>>
>> Looking at r-v8's log [0], it complains that the symbol is missing,
>> indicating it's trying to link with the old version 1.40. I /believe/
>> it's inherited it through r-curl.
>
> If grafting is working as it should, then nghttp2-1.40 should never be
> linked at runtime.  However, it is certainly the case that most things
> (except node-10.22) are *built* against nghttp2-1.40, where the
> aforementioned symbol is missing.
>
> One possible solution might be to update the replacement (graft) for
> _curl_ so that it's *built* against nghttp2-1.41.  Something like this
> (untested):
>
> --8<---------------cut here---------------start------------->8---
> diff --git a/gnu/packages/curl.scm b/gnu/packages/curl.scm
> index 55b7e4393b..bfcb52b678 100644
> --- a/gnu/packages/curl.scm
> +++ b/gnu/packages/curl.scm
> @@ -183,6 +183,9 @@ tunneling, and so on.")
>                (sha256
>                 (base32
>                  "0wlppmx9iry8slh4pqcxj7lwc6fqwnlhh9ri2pcym2rx76a8gwfd"))))
> +    (inputs
> +     `(("nghttp2" ,nghttp2-1.41 "lib")
> +       ,@(alist-delete "nghttp2" (package-inputs curl))))
>      (arguments
>       (substitute-keyword-arguments (package-arguments curl)
>         ((#:phases phases)
> --8<---------------cut here---------------end--------------->8---
>
> Would you like to try this and see if it solves the problem?

I'm afraid this still doesn't solve the problem. AFAIU, grafting the new
curl happens after building r-v8, so at link time it still sees the old
nghttp2 version.

Thinking about this, since the new node essentially uses a new ABI to
link with nghttp2 by requiring a new symbol, this isn't something we can
fix with grafting I believe.

It's possible to make r-curl specifically depend on the new curl like
so:

--8<---------------cut here---------------start------------->8---
diff --git a/gnu/packages/cran.scm b/gnu/packages/cran.scm
index 2c202e8508..16022c695d 100644
--- a/gnu/packages/cran.scm
+++ b/gnu/packages/cran.scm
@@ -1038,7 +1038,7 @@ if(_ca_bundle != NULL) { curl_easy_setopt(handle, CURLOPT_CAINFO, _ca_bundle); }
 " m)))
              #t)))))
     (inputs
-     `(("libcurl" ,curl)
+     `(("libcurl" ,curl-7.71.0)
        ("zlib" ,zlib)))
     (native-inputs
      `(("pkg-config" ,pkg-config)))
diff --git a/gnu/packages/curl.scm b/gnu/packages/curl.scm
index 55b7e4393b..aa103306a6 100644
--- a/gnu/packages/curl.scm
+++ b/gnu/packages/curl.scm
@@ -172,7 +172,7 @@ tunneling, and so on.")
     (inputs (alist-delete "openldap" (package-inputs curl))))))
 
 ;; Replacement package to fix CVE-2020-8169 and CVE-2020-8177.
-(define curl-7.71.0
+(define-public curl-7.71.0
   (package
     (inherit curl)
     (version "7.71.0")
@@ -183,6 +183,9 @@ tunneling, and so on.")
               (sha256
                (base32
                 "0wlppmx9iry8slh4pqcxj7lwc6fqwnlhh9ri2pcym2rx76a8gwfd"))))
+    (inputs
+     `(("nghttp2" ,nghttp2-1.41 "lib")
+       ,@(alist-delete "nghttp2" (package-inputs curl))))
     (arguments
      (substitute-keyword-arguments (package-arguments curl)
        ((#:phases phases)
--8<---------------cut here---------------end--------------->8---

But I'm not sure I like this very much, this is getting a bit messy.

Instead, I'm thinking your suggestion of leaving 'node' at 10.19 for now
(or 10.20, I can try that) and then introduce a 'node-10.22' package
that can be used for Icecat is better. I can do that. How does that
sound?

Thanks,
Pierre

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 519 bytes --]

bug#43228: r-v8 doesn't build since node 10.22 update

[Mark H Weaver]

Hi Pierre,

Pierre Langlois <pierre.langlois@gmx.com> writes:

> Mark H Weaver writes:
>
>> One possible solution might be to update the replacement (graft) for
>> _curl_ so that it's *built* against nghttp2-1.41.  Something like this
>> (untested):
>
> I'm afraid this still doesn't solve the problem. AFAIU, grafting the new
> curl happens after building r-v8, so at link time it still sees the old
> nghttp2 version.

Ah yes, that makes sense.

> Instead, I'm thinking your suggestion of leaving 'node' at 10.19 for now
> (or 10.20, I can try that) and then introduce a 'node-10.22' package
> that can be used for Icecat is better.

That indeed might be the best approach for now.

> I can do that. How does that sound?

Sure, sounds good.  Thanks!

       Mark

bug#43228: r-v8 doesn't build since node 10.22 update

[Pierre Langlois]

[-- Attachment #1.1: Type: text/plain, Size: 970 bytes --]


Mark H Weaver writes:

> Hi Pierre,
>
> Pierre Langlois <pierre.langlois@gmx.com> writes:
>
>> Mark H Weaver writes:
>>
>>> One possible solution might be to update the replacement (graft) for
>>> _curl_ so that it's *built* against nghttp2-1.41.  Something like this
>>> (untested):
>>
>> I'm afraid this still doesn't solve the problem. AFAIU, grafting the new
>> curl happens after building r-v8, so at link time it still sees the old
>> nghttp2 version.
>
> Ah yes, that makes sense.
>
>> Instead, I'm thinking your suggestion of leaving 'node' at 10.19 for now
>> (or 10.20, I can try that) and then introduce a 'node-10.22' package
>> that can be used for Icecat is better.
>
> That indeed might be the best approach for now.
>
>> I can do that. How does that sound?
>
> Sure, sounds good.  Thanks!

Cool, here's a patch to do just that :-). I tried to update node to
10.21 but that still required the newer nghttp2 lib, but it worked for
10.20.

Thanks,
Pierre


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 519 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gnu-node-Downgrade-to-10.20.0.patch --]
[-- Type: text/x-patch, Size: 3680 bytes --]

From eb00ab49df23f7319009a9fec7fc2805016e2e25 Mon Sep 17 00:00:00 2001
From: Pierre Langlois <pierre.langlois@gmx.com>
Date: Sat, 5 Sep 2020 21:05:08 +0100
Subject: [PATCH] gnu: node: Downgrade to 10.20.0.

But keep version 10.22.0 around with a new node-10.22 variable.

* gnu/packages/node.scm (node): Downgrade to 10.22.0.
[inputs]: Downgrade nghttp2 to 1.40.
(node-10.22): New variable.
---
 gnu/packages/node.scm | 44 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/gnu/packages/node.scm b/gnu/packages/node.scm
index ed0b5c4f16..345668fa56 100644
--- a/gnu/packages/node.scm
+++ b/gnu/packages/node.scm
@@ -25,6 +25,7 @@

 (define-module (gnu packages node)
   #:use-module ((guix licenses) #:select (expat))
+  #:use-module ((guix build utils) #:select (alist-replace))
   #:use-module (guix packages)
   #:use-module (guix derivations)
   #:use-module (guix download)
@@ -47,14 +48,14 @@
 (define-public node
   (package
     (name "node")
-    (version "10.22.0")
+    (version "10.20.0")
     (source (origin
               (method url-fetch)
               (uri (string-append "https://nodejs.org/dist/v" version
                                   "/node-v" version ".tar.xz"))
               (sha256
                (base32
-                "1nz18fa550li10r0kzsm28c2rvvq61nq8bqdygip0rmvbi2paxg0"))
+                "0cvjwnl0wkcsyw3kannbdv01s235wrnp11n2s6swzjx95gpichfi"))
               (modules '((guix build utils)))
               (snippet
                `(begin
@@ -186,7 +187,7 @@
        ("http-parser" ,http-parser)
        ("icu4c" ,icu4c)
        ("libuv" ,libuv)
-       ("nghttp2" ,nghttp2-1.41 "lib")
+       ("nghttp2" ,nghttp2 "lib")
        ("openssl" ,openssl)
        ("zlib" ,zlib)))
     (synopsis "Evented I/O for V8 JavaScript")
@@ -200,6 +201,43 @@ devices.")
     (properties '((max-silent-time . 7200)     ;2h, needed on ARM
                   (timeout . 21600)))))        ;6h

+;; TODO: Make this the default node on core-updates.  This cannot be done on
+;; master since this version of node requires a newer nghttp2 library at link
+;; time.
+(define-public node-10.22
+  (package
+    (inherit node)
+    (version "10.22.0")
+    (source (origin
+              (method url-fetch)
+              (uri (string-append "https://nodejs.org/dist/v" version
+                                  "/node-v" version ".tar.xz"))
+              (sha256
+               (base32
+                "1nz18fa550li10r0kzsm28c2rvvq61nq8bqdygip0rmvbi2paxg0"))
+              (modules '((guix build utils)))
+              (snippet
+               `(begin
+                  ;; Remove bundled software.
+                  (for-each delete-file-recursively
+                            '("deps/cares"
+                              "deps/http_parser"
+                              "deps/icu-small"
+                              "deps/nghttp2"
+                              "deps/openssl"
+                              "deps/uv"
+                              "deps/zlib"))
+                  (substitute* "Makefile"
+                    ;; Remove references to bundled software.
+                    (("deps/http_parser/http_parser.gyp") "")
+                    (("deps/uv/include/\\*.h") "")
+                    (("deps/uv/uv.gyp") "")
+                    (("deps/zlib/zlib.gyp") ""))
+                  #t))))
+    (inputs
+     (alist-replace "nghttp2" (list nghttp2-1.41 "lib")
+                    (package-inputs node)))))
+
 (define-public libnode
   (package
     (inherit node)
--
2.28.0

bug#43228: r-v8 doesn't build since node 10.22 update

[Mark H Weaver]

Hi Pierre,

Your new patch looks good to me, but the node-10.22 source field could
be simplified to avoid repeating the unchanged field (especially the
snippet), by inheriting from (package-source node) like this:

--8<---------------cut here---------------start------------->8---
    (source (origin
              (inherit (package-source node))
              (uri (string-append "https://nodejs.org/dist/v" version
                                  "/node-v" version ".tar.xz"))
              (sha256
               (base32
                "1nz18fa550li10r0kzsm28c2rvvq61nq8bqdygip0rmvbi2paxg0"))))
--8<---------------cut here---------------end--------------->8---

Also, it would be great to find a way to fit in the subject line that
10.22 is also being kept as a separate binding, especially since "guix
build node" and most other user commands will still build 10.22.  Maybe
something like this:

  gnu: node: Downgrade to 10.20.0; add separate 'node-10.22' binding.

What do you think?

Anyway, feel free to push this, preferably after incorporating these
suggestions.  If I'm not mistaken, the simplification suggested above
should not change the .drv file, and therefore not entail a rebuild, so
testing it should be very quick.

     Thanks!
       Mark 

bug#43228: r-v8 doesn't build since node 10.22 update

[Pierre Langlois]

[-- Attachment #1: Type: text/plain, Size: 1552 bytes --]

Hi Mark,

Mark H Weaver writes:

> Hi Pierre,
>
> Your new patch looks good to me, but the node-10.22 source field could
> be simplified to avoid repeating the unchanged field (especially the
> snippet), by inheriting from (package-source node) like this:
>
> --8<---------------cut here---------------start------------->8---
>     (source (origin
>               (inherit (package-source node))
>               (uri (string-append "https://nodejs.org/dist/v" version
>                                   "/node-v" version ".tar.xz"))
>               (sha256
>                (base32
>                 "1nz18fa550li10r0kzsm28c2rvvq61nq8bqdygip0rmvbi2paxg0"))))
> --8<---------------cut here---------------end--------------->8---

Oh yeah, that's much better.

>
> Also, it would be great to find a way to fit in the subject line that
> 10.22 is also being kept as a separate binding, especially since "guix
> build node" and most other user commands will still build 10.22.  Maybe
> something like this:
>
>   gnu: node: Downgrade to 10.20.0; add separate 'node-10.22' binding.
>
> What do you think?

Actually, even better, I can split this into two separate commits.

>
> Anyway, feel free to push this, preferably after incorporating these
> suggestions.  If I'm not mistaken, the simplification suggested above
> should not change the .drv file, and therefore not entail a rebuild, so
> testing it should be very quick.

Pushed as 6b7cba0fa897e97b43e76612e3736429426f4d9d and
92db0d39e2aa64be390e86172bd670d98e121c4b, thanks for the review!

Pierre

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 519 bytes --]