Skip to content

test: fix addon tests compilation with OpenSSL 1.1.1 - #44725

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
AdamMajer:openssl_provider_h_test_fix
Sep 22, 2022
Merged

test: fix addon tests compilation with OpenSSL 1.1.1#44725
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
AdamMajer:openssl_provider_h_test_fix

Conversation

@AdamMajer

@AdamMajerAdamMajer commented Sep 19, 2022•
edited by RaisinTen
Loading

Copy link
Copy Markdown
Contributor

openssl/provider.h header is not part of OpenSSL 1.1.1 so do not include it when building with an older instance.

Fixes: #44722

@nodejs-github-botnodejs-github-bot added addons Issues and PRs related to native addons. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Sep 19, 2022
@AdamMajer
test: fix addon tests compilation with OpenSSL 1.1.1
c9419bf
openssl/provider.h header is not part of OpenSSL 1.1.1 so do not
include it when building with an older instance.
Fixes: nodejs#44722
@AdamMajer
AdamMajerforce-pushed the openssl_provider_h_test_fix branch from bd4f5d0 to c9419bfCompareSeptember 19, 2022 12:19
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@tniessentniessen added the openssl Issues and PRs related to the OpenSSL dependency. label Sep 19, 2022
bnoordhuis
bnoordhuis reviewed Sep 20, 2022
View reviewed changes
Comment threadtest/addons/openssl-providers/binding.cc
@AdamMajer@bnoordhuis
Update test/addons/openssl-providers/binding.cc
522f4e8
Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>
@RaisinTen

RaisinTen commented Sep 20, 2022•
edited
Loading

Copy link
Copy Markdown
Member

There's no way to know if this actually fixes the issue because this PR targets main where we use OpenSSL v3. Maybe we could backport #44148 to v16.x-staging (that uses OpenSSL v1) first and then apply this patch to verify?

Also, cc @richardlau since this was added in #44148.

@AdamMajer

AdamMajer commented Sep 20, 2022

Copy link
Copy Markdown
ContributorAuthor

@RaisinTen -- it fixes is 😉

Our default OpenSSL version for openSUSE Tumbleweed is still OpenSSL 1.1.1. This is the reason why this issue came up. The idea is not to break OpenSSL 3.x here while fixing the use-case of external OpenSSL 1.1.1 usage.

https://build.opensuse.org/package/show/devel:languages:nodejs/nodejs18

@richardlau

richardlau commented Sep 20, 2022

Copy link
Copy Markdown
Member

There's no way to know if this actually fixes the issue because this PR targets main where we use OpenSSL v3. Maybe we could backport #44148 to v16.x-staging first and then apply this patch to verify?

The CI, even for main, does test that we can build Node.js against a dynamically linked OpenSSL 1.1.1: https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1804_sharedlibs_openssl111_x64/
That didn't catch this in #44148 for some reason.

@richardlaurichardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 20, 2022
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 20, 2022
@nodejs-github-bot

nodejs-github-bot commented Sep 20, 2022

Copy link
Copy Markdown
Collaborator

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

@RaisinTenRaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 20, 2022
@richardlaurichardlau 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 Sep 22, 2022
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 22, 2022
@nodejs-github-bot
nodejs-github-bot merged commit 4565918 into nodejs:mainSep 22, 2022
@nodejs-github-bot

nodejs-github-bot commented Sep 22, 2022

Copy link
Copy Markdown
Collaborator

Landed in 4565918

RafaelGSS pushed a commit that referenced this pull request Sep 26, 2022
@AdamMajer@RafaelGSS
test: fix addon tests compilation with OpenSSL 1.1.1
8acb425
openssl/provider.h header is not part of OpenSSL 1.1.1 so do not
include it when building with an older instance.
Fixes: #44722
PR-URL: #44725
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@RafaelGSSRafaelGSS mentioned this pull request Sep 26, 2022
Merged
RafaelGSS pushed a commit that referenced this pull request Sep 26, 2022
@AdamMajer@RafaelGSS
test: fix addon tests compilation with OpenSSL 1.1.1
a56cb65
openssl/provider.h header is not part of OpenSSL 1.1.1 so do not
include it when building with an older instance.
Fixes: #44722
PR-URL: #44725
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@juanarbol

juanarbol commented Oct 4, 2022

Copy link
Copy Markdown
Member

Hey, this is fantastic!

I will bale this as "dont-land-on-v16.x", this depends on #44148, which is not landed in the v16.x release branch.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuisbnoordhuisbnoordhuis left review comments
@tniessentniessentniessen approved these changes
@richardlaurichardlaurichardlau approved these changes
@RaisinTenRaisinTenRaisinTen approved these changes

Assignees

No one assigned

Labels

addonsIssues and PRs related to native addons.author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.opensslIssues and PRs related to the OpenSSL dependency.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Question: openssl 1.1.1 supported in 18.x?

7 participants

@AdamMajer@nodejs-github-bot@RaisinTen@richardlau@juanarbol@bnoordhuis@tniessen