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.

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

tls: fix macro to check NPN feature #11655

Closed
wants to merge 2 commits into from
Closed

tls: fix macro to check NPN feature #11655

wants to merge 2 commits into from

Conversation

shigeki
Copy link
Contributor

@shigeki shigeki commented Mar 2, 2017

In order to check if NPN feature is enabled, use #ifndef OPENSSL_NO_NEXTPROTONEG rather than #ifdef OPENSSL_NPN_NEGOTIATED because the former is used in ssl.h.
An ALPN test also needs NPN feature to run and it was fixed. This also change the messages when ALPN and NPN tests are skipped.

Fix #11650

Build and tests are made with an additional commit of

diff --git a/node.gyp b/node.gyp
index 673a1d1..6a26cb1 100644
--- a/node.gyp
+++ b/node.gyp
@@ -257,6 +257,7 @@
         'NODE_WANT_INTERNALS=1',
         # Warn when using deprecated V8 APIs.
         'V8_DEPRECATION_WARNINGS=1',
+        'OPENSSL_NO_NEXTPROTONEG'
       ],

which is not included in this PR. I also made build and test in my local host and also submitted a CI job to https://ci.nodejs.org/job/node-test-commit/8210/ with this additional commit.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

tls, test

@nodejs/crypto

@shigeki
tls: fix macro to check NPN feature
0850e69 
In order to check if NPN feature is enabled, use
`#ifndef OPENSSL_NO_NEXTPROTONEG` rather than
`#ifdef OPENSSL_NPN_NEGOTIATED` because the former is used in ssl.h.
@shigeki
test: fix tests when npn feature is disabled.
c29150d 
ALPN test needs NPN feature to run. It also change the messages when
ALPN and NPN tests are skipped.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Mar 2, 2017
@shigeki shigeki added build Issues and PRs related to build files or the CI. openssl Issues and PRs related to the OpenSSL dependency. test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem. labels Mar 2, 2017
@shigeki shigeki mentioned this pull request Mar 2, 2017
Closed
mhdawson
mhdawson approved these changes Mar 2, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mscdex mscdex removed crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests. labels Mar 2, 2017
indutny
indutny approved these changes Mar 3, 2017
View reviewed changes
Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM

@shigeki
Copy link
Contributor Author

shigeki commented Mar 3, 2017

Some flaky tests are resolved in the current master.
A new CI job is submitted again in https://ci.nodejs.org/job/node-test-commit/8229/ and all is green.

shigeki added a commit that referenced this pull request Mar 3, 2017
@shigeki
tls: fix macro to check NPN feature
02c98f4 
In order to check if NPN feature is enabled, use
`#ifndef OPENSSL_NO_NEXTPROTONEG` rather than
`#ifdef OPENSSL_NPN_NEGOTIATED` because the former is used in ssl.h.

Fixes: #11650
PR-URL: #11655
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
shigeki added a commit that referenced this pull request Mar 3, 2017
@shigeki
test: fix tests when npn feature is disabled.
1824bbb 
ALPN test needs NPN feature to run. It also change the messages when
ALPN and NPN tests are skipped.

Fixes: #11650
PR-URL: #11655
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
@shigeki
Copy link
Contributor Author

shigeki commented Mar 3, 2017

Landed in 02c98f4 and 1824bbb. Thanks.

@shigeki shigeki closed this Mar 3, 2017
addaleax pushed a commit that referenced this pull request Mar 5, 2017
@shigeki @addaleax
tls: fix macro to check NPN feature
d6ac192 
In order to check if NPN feature is enabled, use
`#ifndef OPENSSL_NO_NEXTPROTONEG` rather than
`#ifdef OPENSSL_NPN_NEGOTIATED` because the former is used in ssl.h.

Fixes: #11650
PR-URL: #11655
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
addaleax pushed a commit that referenced this pull request Mar 5, 2017
@shigeki @addaleax
test: fix tests when npn feature is disabled.
8377374 
ALPN test needs NPN feature to run. It also change the messages when
ALPN and NPN tests are skipped.

Fixes: #11650
PR-URL: #11655
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
@evanlucas evanlucas mentioned this pull request Mar 8, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Apr 17, 2017
@shigeki @MylesBorins
tls: fix macro to check NPN feature
3055ebb 
In order to check if NPN feature is enabled, use
`#ifndef OPENSSL_NO_NEXTPROTONEG` rather than
`#ifdef OPENSSL_NPN_NEGOTIATED` because the former is used in ssl.h.

Fixes: #11650
PR-URL: #11655
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 17, 2017
@shigeki @MylesBorins
test: fix tests when npn feature is disabled.
c5363b0 
ALPN test needs NPN feature to run. It also change the messages when
ALPN and NPN tests are skipped.

Fixes: #11650
PR-URL: #11655
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
@MylesBorins
Copy link
Contributor

I've gone ahead and backported this to v6.x-staging as it appears useful. Please let me know if it shouldn't have landed

MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@shigeki @MylesBorins
tls: fix macro to check NPN feature
b1ddf11 
In order to check if NPN feature is enabled, use
`#ifndef OPENSSL_NO_NEXTPROTONEG` rather than
`#ifdef OPENSSL_NPN_NEGOTIATED` because the former is used in ssl.h.

Fixes: #11650
PR-URL: #11655
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@shigeki @MylesBorins
test: fix tests when npn feature is disabled.
7cc4645 
ALPN test needs NPN feature to run. It also change the messages when
ALPN and NPN tests are skipped.

Fixes: #11650
PR-URL: #11655
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@shigeki @andrew749
tls: fix macro to check NPN feature
3e8231c 
In order to check if NPN feature is enabled, use
`#ifndef OPENSSL_NO_NEXTPROTONEG` rather than
`#ifdef OPENSSL_NPN_NEGOTIATED` because the former is used in ssl.h.

Fixes: nodejs/node#11650
PR-URL: nodejs/node#11655
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@shigeki @andrew749
test: fix tests when npn feature is disabled.
c752f20 
ALPN test needs NPN feature to run. It also change the messages when
ALPN and NPN tests are skipped.

Fixes: nodejs/node#11650
PR-URL: nodejs/node#11655
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@indutny indutny indutny approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. openssl Issues and PRs related to the OpenSSL dependency. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Node fails to compile if OPENSSL_NO_NEXTPROTONEG is set
8 participants
@shigeki @MylesBorins @indutny @bnoordhuis @jasnell @mhdawson @mscdex @nodejs-github-bot