src: gate all quic behind disabled-by-default compile flag by jasnell · Pull Request #57142 · nodejs/node · GitHub

Due to quictls/openssl@93ae85b it is clear that we will need to revert back to using OpenSSL's official releases. This means we will be forced to re-implement at least part of the underlying QUIC implementation to use different crypto APIs. For that reason, this PR disables building any of the QUIC support by default and introduces a new compile time flag.

/cc @mhdawson @ljharb @mcollina @anonrig @Qard @richardlau

Review requested:

@nodejs/gyp

can not review the contents, but rubberstamping based on the intentions (please ensure someone has reviewed the contents before landing tho)

LGTM

Fast-track has been requested by @anonrig. Please 👍 to approve.

Needed to push one additional change to handle the --experimental-quic CLI flag correctly. Without the compile flag it is allowed but a non-op. With the flag it becomes functional.

Due to quictls/openssl@93ae85b it is clear that we will need to revert back to using OpenSSL's official releases. This means we will be forced to re-implement at least part of the underlying QUIC implementation to use different crypto APIs. For that reason, this PR disables building any of the QUIC support by default and introduces a new compile time flag.

CI: https://ci.nodejs.org/job/node-test-pull-request/65335/

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.37%. Comparing base (f6ce486) to head (029f974).
Report is 373 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57142      +/-   ##
==========================================
+ Coverage   88.54%   90.37%   +1.83%     
==========================================
  Files         665      628      -37     
  Lines      193408   184007    -9401     
  Branches    36961    35952    -1009     
==========================================
- Hits       171250   166304    -4946     
+ Misses      14795    10869    -3926     
+ Partials     7363     6834     -529

Files with missing lines	Coverage Δ
src/node_options.cc	87.37% <ø> (ø)
src/node_options.h	98.32% <ø> (-0.01%)	⬇️

... and 146 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Change `tools/dep_updaters/update-openssl.sh` to fetch updates from official OpenSSL. PR-URL: #57301 Refs: #57142 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Update the instructions for maintaining OpenSSL in the Node.js source tree to reflect switching back from the quictls fork of OpenSSL back to official OpenSSL. PR-URL: nodejs#57413 Refs: nodejs#57301 Refs: nodejs#57142 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

This commit didn't land cleanly on v22.x-staging. It requires a manual backport, so I'm adding a backport-requested label.

Update the instructions for maintaining OpenSSL in the Node.js source tree to reflect switching back from the quictls fork of OpenSSL back to official OpenSSL. PR-URL: #57413 Refs: #57301 Refs: #57142 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Change `tools/dep_updaters/update-openssl.sh` to fetch updates from official OpenSSL. PR-URL: #57301 Refs: #57142 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Update the instructions for maintaining OpenSSL in the Node.js source tree to reflect switching back from the quictls fork of OpenSSL back to official OpenSSL. PR-URL: #57413 Refs: #57301 Refs: #57142 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Change `tools/dep_updaters/update-openssl.sh` to fetch updates from official OpenSSL. PR-URL: #57301 Refs: #57142 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Update the instructions for maintaining OpenSSL in the Node.js source tree to reflect switching back from the quictls fork of OpenSSL back to official OpenSSL. PR-URL: #57413 Refs: #57301 Refs: #57142 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

`parallel/test-config-json-schema` compares to a generated fixture that assumes that Node.js was built with default configuration settings. Skip the test if non-default quic is enabled (`configure --with-quic`). Refs: #57016 Refs: #57142 PR-URL: #57225 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

Change `tools/dep_updaters/update-openssl.sh` to fetch updates from official OpenSSL. PR-URL: #57301 Refs: #57142 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Update the instructions for maintaining OpenSSL in the Node.js source tree to reflect switching back from the quictls fork of OpenSSL back to official OpenSSL. PR-URL: #57413 Refs: #57301 Refs: #57142 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

`parallel/test-config-json-schema` compares to a generated fixture that assumes that Node.js was built with default configuration settings. Skip the test if non-default quic is enabled (`configure --with-quic`). Refs: #57016 Refs: #57142 PR-URL: #57225 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

Change `tools/dep_updaters/update-openssl.sh` to fetch updates from official OpenSSL. PR-URL: #57301 Refs: #57142 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Update the instructions for maintaining OpenSSL in the Node.js source tree to reflect switching back from the quictls fork of OpenSSL back to official OpenSSL. PR-URL: #57413 Refs: #57301 Refs: #57142 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

`parallel/test-config-json-schema` compares to a generated fixture that assumes that Node.js was built with default configuration settings. Skip the test if non-default quic is enabled (`configure --with-quic`). Refs: #57016 Refs: #57142 PR-URL: #57225 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. labels Feb 19, 2025

This comment was marked as outdated.

Sign in to view

anonrig approved these changes Feb 19, 2025

View reviewed changes

jasnell force-pushed the jasnell/quic-sad-face branch from fb75531 to ea2bf13 Compare February 19, 2025 22:02

This comment was marked as outdated.

Sign in to view

ljharb approved these changes Feb 19, 2025

View reviewed changes

marco-ippolito approved these changes Feb 19, 2025

View reviewed changes

mhdawson approved these changes Feb 19, 2025

View reviewed changes

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 19, 2025

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 19, 2025

This comment was marked as duplicate.

Sign in to view

anonrig added the fast-track PRs that do not need to wait for 48 hours to land. label Feb 19, 2025

legendecas approved these changes Feb 19, 2025

View reviewed changes

jasnell added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Feb 19, 2025

jasnell force-pushed the jasnell/quic-sad-face branch 2 times, most recently from fa8cf59 to 95bd1c3 Compare February 19, 2025 23:47

This comment was marked as outdated.

Sign in to view

jasnell added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 19, 2025

richardlau approved these changes Feb 19, 2025

View reviewed changes

anonrig approved these changes Feb 20, 2025

View reviewed changes

jasnell force-pushed the jasnell/quic-sad-face branch from 95bd1c3 to 029f974 Compare February 20, 2025 00:20

RafaelGSS mentioned this pull request Apr 8, 2025

v22.x-staging cherry-pick status #57799

Closed

RafaelGSS added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Apr 11, 2025

src: gate all quic behind disabled-by-default compile flag by jasnell · Pull Request #57142 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: gate all quic behind disabled-by-default compile flag - #57142

src: gate all quic behind disabled-by-default compile flag#57142
jasnell wants to merge 1 commit into
nodejs:mainfrom
jasnell:jasnell/quic-sad-face

jasnell commented Feb 19, 2025 •
edited
Loading

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 19, 2025

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

ljharb left a comment

Uh oh!

mhdawson left a comment

Uh oh!

This comment was marked as duplicate.

github-actions Bot commented Feb 19, 2025

Uh oh!

jasnell commented Feb 19, 2025

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Feb 20, 2025

Uh oh!

codecov Bot commented Feb 20, 2025 •
edited
Loading

Uh oh!

Uh oh!

RafaelGSS commented Apr 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

src: gate all quic behind disabled-by-default compile flag by jasnell · Pull Request #57142 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: gate all quic behind disabled-by-default compile flag - #57142

src: gate all quic behind disabled-by-default compile flag#57142jasnell wants to merge 1 commit intonodejs:mainnodejs/node:mainfrom jasnell:jasnell/quic-sad-facejasnell/node:jasnell/quic-sad-faceCopy head branch name to clipboard

Conversation

jasnell commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 19, 2025

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as duplicate.

github-actions Bot commented Feb 19, 2025

Uh oh!

jasnell commented Feb 19, 2025

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Feb 20, 2025

Uh oh!

codecov Bot commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

RafaelGSS commented Apr 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

src: gate all quic behind disabled-by-default compile flag#57142
jasnell wants to merge 1 commit into
nodejs:mainfrom
jasnell:jasnell/quic-sad-face

jasnell commented Feb 19, 2025 •
edited
Loading

codecov Bot commented Feb 20, 2025 •
edited
Loading