Navigation Menu

Skip to content
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

Jump to bottom

test: fix addon tests compilation with OpenSSL 1.1.1 #44725

Merged
merged 2 commits into from Sep 22, 2022
Merged

test: fix addon tests compilation with OpenSSL 1.1.1 #44725

merged 2 commits into from Sep 22, 2022

Conversation

AdamMajer
Copy link
Contributor

@AdamMajer AdamMajer commented Sep 19, 2022 •
edited by RaisinTen

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

This comment was marked as outdated.

Sign in to view
@tniessen tniessen added the openssl Issues and PRs related to the OpenSSL dependency. label Sep 19, 2022
@AdamMajer @bnoordhuis
Update test/addons/openssl-providers/binding.cc
522f4e8 
Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>
@RaisinTen
Copy link
Contributor

RaisinTen commented Sep 20, 2022 •
edited

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
Contributor Author

@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

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.

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 20, 2022
@github-actions github-actions bot 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

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

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 20, 2022
@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 Sep 22, 2022
@nodejs-github-bot nodejs-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:main Sep 22, 2022
@nodejs-github-bot
Copy link
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>
@RafaelGSS RafaelGSS 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 <rlau@redhat.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
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
Copy link
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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis left review comments

@tniessen tniessen tniessen approved these changes

@richardlau richardlau richardlau approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

Assignees
No one assigned
Labels
addons Issues and PRs related to native addons. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. openssl Issues and PRs related to the OpenSSL dependency. test Issues 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