Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: reintroduce ngtcp2 and nghttp3 #37682

Closed
wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Mar 9, 2021

This builds on #37601. The first four commits are from that PR and will be dropped once that lands.

Reintroduce the ngtcp2 and nghttp3 dependencies, building those by default if the vendored-in openssl is used (with the QUIC apis) or the shared-openssl defines OPENSSL_INFO_QUIC (signaling QUIC support).

The Node.js version metadata is updated to reflect whether ngtcp2 and nghttp3 are present.

This does not include the rest of the QUIC implementation yet. This is just adding back the dependencies.

@jasnell jasnell added blocked PRs that are blocked by other issues or PRs. quic Issues and PRs related to the QUIC implementation / HTTP/3. labels Mar 9, 2021
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. labels Mar 9, 2021
@jasnell jasnell removed the blocked PRs that are blocked by other issues or PRs. label Mar 15, 2021
@jasnell jasnell force-pushed the the-quic-comeback-tour-act-2 branch from 3c719e9 to 2e0be08 Compare March 15, 2021 14:37
@jasnell jasnell removed the needs-ci PRs that need a full CI run. label Mar 15, 2021
@nodejs-github-bot

This comment has been minimized.

@jasnell
Copy link
Member Author

jasnell commented Mar 15, 2021

@danbev @addaleax @mhdawson @mcollina @nodejs/quic ... please take a look

@targos
Copy link
Member

targos commented Mar 15, 2021

Could you add documentation on how to update these dependencies?

@jasnell
Copy link
Member Author

jasnell commented Mar 15, 2021

@targos .. it's already there. Take a look at the deps/ngtcp2/README.md

@targos
Copy link
Member

targos commented Mar 15, 2021

@jasnell oh sorry, I assumed it would be located in doc/guides like for other dependencies.

@jasnell
Copy link
Member Author

jasnell commented Mar 15, 2021

I was considering that but I'd really like to start keeping the documentation for that with the dependencies themselves. I can move it in to doc/guides if necessary

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@jasnell jasnell force-pushed the the-quic-comeback-tour-act-2 branch from 9c33a9c to eb345ad Compare March 15, 2021 21:21
@nodejs-github-bot
Copy link
Collaborator

@jasnell jasnell force-pushed the the-quic-comeback-tour-act-2 branch from eb345ad to 4bc502a Compare March 15, 2021 21:57
Copy link
Contributor

@danbev danbev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally using the following patch to resolve the conflicts.

patch
diff --git a/node.gypi b/node.gypi
index f9dba2d4bd..7569705733 100644
--- a/node.gypi
+++ b/node.gypi
@@ -365,8 +365,18 @@
           # Set 1.0.0 as the API compability level to avoid the
           # deprecation warnings when using OpenSSL 3.0.
 	  'defines': ['OPENSSL_API_COMPAT=0x10000000L'],
-	}]]
-
+	}],
+        [ 'openssl_quic=="true" and node_shared_ngtcp2=="false"', {
+          'dependencies': [
+            './deps/ngtcp2/ngtcp2.gyp:ngtcp2'
+          ]
+        }],
+        [ 'openssl_quic=="true" and node_shared_nghttp3=="false"', {
+          'dependencies': [
+            './deps/ngtcp2/ngtcp2.gyp:nghttp3'
+          ]
+        }],
+      ]
     }, {
       'defines': [ 'HAVE_OPENSSL=0' ]
     }],
diff --git a/test/common/index.js b/test/common/index.js
index 89c4fcaaa9..894c8025e7 100644
--- a/test/common/index.js
+++ b/test/common/index.js
@@ -54,6 +54,8 @@ const hasCrypto = Boolean(process.versions.openssl) &&
 const hasOpenSSL3 = hasCrypto &&
     require('crypto').constants.OPENSSL_VERSION_NUMBER >= 805306368;
 
+const hasQuic = hasCrypto && !!process.config.variables.openssl_quic;
+
 // Check for flags. Skip this for workers (both, the `cluster` module and
 // `worker_threads`) and child processes.
 // If the binary was built without-ssl then the crypto flags are
@@ -730,6 +732,7 @@ const common = {
   hasIntl,
   hasCrypto,
   hasOpenSSL3,
+  hasQuic,
   hasMultiLocalhost,
   invalidArgTypeHelper,
   isAIX,

If the shared openssl does not have the `OPENSSL_INFO_QUIC` define,
then it definitely does not have the QUIC apis. This is only a partially
accurate check because it does not detect if the shared openssl was
actually *built* without the OPENSSL_NO_QUIC define set.

Signed-off-by: James M Snell <jasnell@gmail.com>
Reintroduces the ngtcp2 and nghttp3 dependencies, building those by
default if the vendored-in openssl (with QUIC support) is used or the
shared openssl defines `OPENSSL_INFO_QUIC`.

Upates the version metadata to reflect whether ngtcp2 and nghttp3 are
present.

ngtcp2 as of
ngtcp2/ngtcp2@2381f7f
nghttp3 as of
ngtcp2/nghttp3@66ad30f

Signed-off-by: James M Snell <jasnell@gmail.com>
@jasnell jasnell force-pushed the the-quic-comeback-tour-act-2 branch from 4bc502a to d7fc8f7 Compare March 18, 2021 18:46
@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 18, 2021
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jasnell
Copy link
Member Author

jasnell commented Mar 18, 2021

I'll be landing this once the final CI test completes (assuming it does, it's going sloooooooow).

The next step will be reintroducing the QUIC implementation itself, which will be coming likely (hopefully) in the next two weeks.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@jasnell
Copy link
Member Author

jasnell commented Mar 19, 2021

Landed in 65e8864...43f599b ... as a reminder, this is just the dependencies. The actual quic implementation will be coming soon

@jasnell jasnell closed this Mar 19, 2021
jasnell added a commit that referenced this pull request Mar 19, 2021
If the shared openssl does not have the `OPENSSL_INFO_QUIC` define,
then it definitely does not have the QUIC apis. This is only a partially
accurate check because it does not detect if the shared openssl was
actually *built* without the OPENSSL_NO_QUIC define set.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37682
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
jasnell added a commit that referenced this pull request Mar 19, 2021
Reintroduces the ngtcp2 and nghttp3 dependencies, building those by
default if the vendored-in openssl (with QUIC support) is used or the
shared openssl defines `OPENSSL_INFO_QUIC`.

Upates the version metadata to reflect whether ngtcp2 and nghttp3 are
present.

ngtcp2 as of
ngtcp2/ngtcp2@2381f7f
nghttp3 as of
ngtcp2/nghttp3@66ad30f

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37682
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
ruyadorno pushed a commit that referenced this pull request Mar 24, 2021
If the shared openssl does not have the `OPENSSL_INFO_QUIC` define,
then it definitely does not have the QUIC apis. This is only a partially
accurate check because it does not detect if the shared openssl was
actually *built* without the OPENSSL_NO_QUIC define set.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37682
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
ruyadorno pushed a commit that referenced this pull request Mar 24, 2021
Reintroduces the ngtcp2 and nghttp3 dependencies, building those by
default if the vendored-in openssl (with QUIC support) is used or the
shared openssl defines `OPENSSL_INFO_QUIC`.

Upates the version metadata to reflect whether ngtcp2 and nghttp3 are
present.

ngtcp2 as of
ngtcp2/ngtcp2@2381f7f
nghttp3 as of
ngtcp2/nghttp3@66ad30f

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37682
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. quic Issues and PRs related to the QUIC implementation / HTTP/3.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants