Skip to content

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

Closed
jasnell wants to merge 1 commit into
nodejs:mainfrom
jasnell:jasnell/quic-sad-face
Closed

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

Conversation

@jasnell

@jasnelljasnell commented Feb 19, 2025•
edited
Loading

Copy link
Copy Markdown
Member

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

@nodejs-github-bot

nodejs-github-bot commented Feb 19, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp

@nodejs-github-botnodejs-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
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jasnell
jasnellforce-pushed the jasnell/quic-sad-face branch from fb75531 to ea2bf13CompareFebruary 19, 2025 22:02
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
ljharb
ljharb approved these changes Feb 19, 2025
View reviewed changes

@ljharbljharb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

mhdawson
mhdawson approved these changes Feb 19, 2025
View reviewed changes

@mhdawsonmhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@mhdawsonmhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 19, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 19, 2025
@nodejs-github-bot

This comment was marked as duplicate.

Sign in to view
@anonriganonrig added the fast-track PRs that do not need to wait for 48 hours to land. label Feb 19, 2025
@github-actions

github-actionsBot commented Feb 19, 2025

Copy link
Copy Markdown
Contributor

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

@jasnelljasnell added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 19, 2025
@jasnell
jasnellforce-pushed the jasnell/quic-sad-face branch 2 times, most recently from fa8cf59 to 95bd1c3CompareFebruary 19, 2025 23:47
@jasnell

jasnell commented Feb 19, 2025

Copy link
Copy Markdown
MemberAuthor

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.

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jasnelljasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 19, 2025
@jasnell
src: gate all quic behind disabled-by-default compile flag
029f974
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.
@jasnell
jasnellforce-pushed the jasnell/quic-sad-face branch from 95bd1c3 to 029f974CompareFebruary 20, 2025 00:20
@nodejs-github-bot

nodejs-github-bot commented Feb 20, 2025

Copy link
Copy Markdown
Collaborator

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

@codecov

codecovBot commented Feb 20, 2025•
edited
Loading

Copy link
Copy Markdown

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 linesCoverage Δ
src/node_options.cc87.37% <ø> (ø)
src/node_options.h98.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.

aduh95 pushed a commit that referenced this pull request Apr 2, 2025
@richardlau@aduh95
tools: switch back to official OpenSSL
0a39006
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>
aduh95 pushed a commit that referenced this pull request Apr 3, 2025
@richardlau@aduh95
tools: switch back to official OpenSSL
7e099f5
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>
@RafaelGSSRafaelGSS mentioned this pull request Apr 8, 2025
Closed
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Apr 8, 2025
@richardlau@RafaelGSS
doc: update maintaining-openssl.md for openssl
44072cc
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>
@RafaelGSSRafaelGSS added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Apr 11, 2025
@RafaelGSS

RafaelGSS commented Apr 11, 2025

Copy link
Copy Markdown
Member

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

RafaelGSS pushed a commit that referenced this pull request Apr 14, 2025
@richardlau@RafaelGSS
doc: update maintaining-openssl.md for openssl
60eacb0
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>
aduh95 pushed a commit that referenced this pull request Apr 14, 2025
@richardlau@aduh95
doc: update maintaining-openssl.md for openssl
a5c84e3
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>
aduh95 pushed a commit that referenced this pull request Apr 15, 2025
@richardlau@aduh95
doc: update maintaining-openssl.md for openssl
bcf7681
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>
RafaelGSS pushed a commit that referenced this pull request Apr 16, 2025
@richardlau@RafaelGSS
tools: switch back to official OpenSSL
64d9d96
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>
RafaelGSS pushed a commit that referenced this pull request Apr 16, 2025
@richardlau@RafaelGSS
doc: update maintaining-openssl.md for openssl
85bfba5
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>
RafaelGSS pushed a commit that referenced this pull request Apr 17, 2025
@richardlau@RafaelGSS
tools: switch back to official OpenSSL
502bfaf
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>
RafaelGSS pushed a commit that referenced this pull request Apr 17, 2025
@richardlau@RafaelGSS
doc: update maintaining-openssl.md for openssl
2dd72c6
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>
RafaelGSS pushed a commit that referenced this pull request May 1, 2025
@richardlau@RafaelGSS
test: skip test-config-json-schema with quic
4fc9e01
`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>
RafaelGSS pushed a commit that referenced this pull request May 1, 2025
@richardlau@RafaelGSS
tools: switch back to official OpenSSL
d85a9b8
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>
RafaelGSS pushed a commit that referenced this pull request May 1, 2025
@richardlau@RafaelGSS
doc: update maintaining-openssl.md for openssl
2d306e7
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>
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
@richardlau@RafaelGSS
test: skip test-config-json-schema with quic
360b283
`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>
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
@richardlau@RafaelGSS
tools: switch back to official OpenSSL
0fb8756
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>
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
@richardlau@RafaelGSS
doc: update maintaining-openssl.md for openssl
b088332
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>
aduh95 pushed a commit that referenced this pull request May 6, 2025
@richardlau@aduh95
test: skip test-config-json-schema with quic
89f218d
`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>
aduh95 pushed a commit that referenced this pull request May 6, 2025
@richardlau@aduh95
test: skip test-config-json-schema with quic
824dac0
`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>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
@richardlau@RafaelGSS
test: skip test-config-json-schema with quic
50d6cb9
`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>
aduh95 pushed a commit that referenced this pull request May 16, 2025
@richardlau@aduh95
test: skip test-config-json-schema with quic
c5625ab
`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>
aduh95 pushed a commit that referenced this pull request May 17, 2025
@richardlau@aduh95
test: skip test-config-json-schema with quic
17f7142
`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>
aduh95 pushed a commit that referenced this pull request May 18, 2025
@richardlau@aduh95
test: skip test-config-json-schema with quic
1c4fac2
`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>
aduh95 pushed a commit that referenced this pull request May 19, 2025
@richardlau@aduh95
test: skip test-config-json-schema with quic
1be5a8c
`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>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharbljharbljharb approved these changes
@mcollinamcollinamcollina approved these changes
@QardQardQard approved these changes
@anonriganonriganonrig approved these changes
@joyeecheungjoyeecheungjoyeecheung approved these changes
@richardlaurichardlaurichardlau approved these changes
@legendecaslegendecaslegendecas approved these changes
@trivikrtrivikrtrivikr approved these changes
@marco-ippolitomarco-ippolitomarco-ippolito approved these changes
+1 more reviewer
@mhdawsonmhdawsonmhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.backport-requested-v22.xPRs awaiting manual backport to the v22.x-staging branch.buildIssues and PRs related to build files or the CI.fast-trackPRs that do not need to wait for 48 hours to land.needs-ciPRs that need a full CI run.toolsIssues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

13 participants

@jasnell@nodejs-github-bot@RafaelGSS@ljharb@mcollina@Qard@anonrig@joyeecheung@richardlau@legendecas@mhdawson@trivikr@marco-ippolito