Skip to content

crypto: fix inclusion of OPENSSL_IS_BORINGSSL define #58845

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

Merged
merged 2 commits into from
Jul 1, 2025

Conversation

codebytere
Copy link
Member

Refs #58387.

Fixes OPENSSL_IS_BORINGSSL not being defined in cases where it should be due to missing include. See relevant BoringSSL file

@codebytere codebytere requested review from jasnell and tniessen June 26, 2025 12:59
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/config

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jun 26, 2025
@codebytere
Copy link
Member Author

codebytere commented Jun 26, 2025

@jasnell looks like openssl itself doesn't have base.h - looks like to ensure it works I just need to include a file that normal OpenSSL does export, bc in BoringSSL they all include base.h. Do you have a preference?

See ex:

Screenshot 2025-06-26 at 3 17 36 PM

Copy link

codecov bot commented Jun 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.10%. Comparing base (b4c5fb4) to head (a0c73cd).
Report is 33 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58845      +/-   ##
==========================================
+ Coverage   90.08%   90.10%   +0.02%     
==========================================
  Files         640      640              
  Lines      188446   188426      -20     
  Branches    36960    36953       -7     
==========================================
+ Hits       169757   169786      +29     
+ Misses      11412    11350      -62     
- Partials     7277     7290      +13     
Files with missing lines Coverage Δ
src/node_config.cc 96.55% <ø> (ø)

... and 46 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.

@jasnell
Copy link
Member

jasnell commented Jun 27, 2025

No particular preference @codebytere ... I'd trust whatever you think needs to be done and whatever makes it work for you

@codebytere codebytere added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Jun 27, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 27, 2025
@nodejs-github-bot

This comment was marked as outdated.

Co-authored-by: Anna Henningsen <github@addaleax.net>
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@codebytere codebytere added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 1, 2025
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 1, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/58845
✔  Done loading data for nodejs/node/pull/58845
----------------------------------- PR info ------------------------------------
Title      crypto: fix inclusion of `OPENSSL_IS_BORINGSSL` define (#58845)
Author     Shelley Vohr <shelley.vohr@gmail.com> (@codebytere)
Branch     codebytere:fix-have-crypto -> nodejs:main
Labels     c++
Commits    2
 - crypto: fix inclusion of OPENSSL_IS_BORINGSSL define
 - Update src/node_config.cc
Committers 1
 - Shelley Vohr <shelley.vohr@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/58845
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/58845
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 26 Jun 2025 12:59:57 GMT
   ✔  Approvals: 5
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/58845#pullrequestreview-2970781596
   ✔  - Juan José Arboleda (@juanarbol): https://github.com/nodejs/node/pull/58845#pullrequestreview-2962886452
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/58845#pullrequestreview-2967069855
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/58845#pullrequestreview-2967665439
   ✔  - Ethan Arrowood (@Ethan-Arrowood): https://github.com/nodejs/node/pull/58845#pullrequestreview-2972085344
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-06-30T18:46:52Z: https://ci.nodejs.org/job/node-test-pull-request/67753/
- Querying data for job/node-test-pull-request/67753/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 58845
From https://github.com/nodejs/node
 * branch                  refs/pull/58845/merge -> FETCH_HEAD
✔  Fetched commits as 2022f832a98b..a0c73cda8176
--------------------------------------------------------------------------------
[main 77a12159c6] crypto: fix inclusion of OPENSSL_IS_BORINGSSL define
 Author: Shelley Vohr <shelley.vohr@gmail.com>
 Date: Thu Jun 26 14:57:05 2025 +0200
 1 file changed, 4 insertions(+)
[main baccb14ceb] Update src/node_config.cc
 Author: Shelley Vohr <shelley.vohr@gmail.com>
 Date: Mon Jun 30 10:16:10 2025 +0200
 1 file changed, 1 insertion(+), 1 deletion(-)
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
Rebasing (2/4)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
crypto: fix inclusion of OPENSSL_IS_BORINGSSL define

PR-URL: #58845
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>

[detached HEAD 9574c03bfe] crypto: fix inclusion of OPENSSL_IS_BORINGSSL define
Author: Shelley Vohr <shelley.vohr@gmail.com>
Date: Thu Jun 26 14:57:05 2025 +0200
1 file changed, 4 insertions(+)
Rebasing (3/4)
Rebasing (4/4)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
Update src/node_config.cc

Co-authored-by: Anna Henningsen <github@addaleax.net>
PR-URL: #58845
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>

[detached HEAD 243b9df2f2] Update src/node_config.cc
Author: Shelley Vohr <shelley.vohr@gmail.com>
Date: Mon Jun 30 10:16:10 2025 +0200
1 file changed, 1 insertion(+), 1 deletion(-)
Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/15993773799

@richardlau richardlau removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jul 1, 2025
@richardlau richardlau added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jul 1, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 1, 2025
@nodejs-github-bot nodejs-github-bot merged commit a7a37c3 into nodejs:main Jul 1, 2025
68 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in a7a37c3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants