Skip to content

test: fix addon tests compilation with OpenSSL 1.1.1#44725

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.

Sign up for GitHub

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

Jump to bottom
Merged
nodejs-github-bot merged 2 commits into from
Sep 22, 2022
Merged

test: fix addon tests compilation with OpenSSL 1.1.1 #44725

nodejs-github-bot merged 2 commits into from
Sep 22, 2022

Conversation

@AdamMajer
Copy link
Contributor

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

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
@AdamMajerAdamMajerforce-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
@RaisinTen
Copy link
Member

RaisinTen commented Sep 20, 2022•
edited
Loading

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
Copy link
ContributorAuthor

AdamMajer commented Sep 20, 2022

@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
Copy link
Member

richardlau commented Sep 20, 2022

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
Copy link
Collaborator

nodejs-github-bot commented Sep 20, 2022

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-botnodejs-github-bot merged commit 4565918 into nodejs:mainSep 22, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 22, 2022

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 <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
@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
54cd2f4
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 <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
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 <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
@juanarbol
Copy link
Member

juanarbol commented Oct 4, 2022

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