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

Node fails to compile if OPENSSL_NO_NEXTPROTONEG is set #11650

Closed
ygalblum opened this issue Mar 2, 2017 · 4 comments
Closed

Node fails to compile if OPENSSL_NO_NEXTPROTONEG is set #11650

ygalblum opened this issue Mar 2, 2017 · 4 comments
Labels
build Issues and PRs related to build files or the CI. openssl Issues and PRs related to the OpenSSL dependency.

Comments

@ygalblum
Copy link

ygalblum commented Mar 2, 2017

  • Version: v7.6.0 (but it is very old and exists also in v0.12.7)
  • Platform: 3.4.11-rt19 test: don't remove empty.txt on win32 #2 SMP PREEMPT Wed Mar 1 12:43:11 IST 2017 armv7l GNU/Linux
  • Subsystem: node_crypto.cc

When compiling NodeJS with an external OpenSSL library that was compiled with the flag OPENSSL_NO_NEXTPROTONEG, the compilation of node_crypto.cc fails. The reason is that instead of checking this preprocessor flag, node checks whether OPENSSL_NPN_NEGOTIATED is defined. However, the latter exists regardless to the former.
I think that the solution is replacing between the dependencies. But I cannot say for sure (if I were sure I can verify this fix, I'd offer the patch)

@mscdex mscdex added build Issues and PRs related to build files or the CI. openssl Issues and PRs related to the OpenSSL dependency. labels Mar 2, 2017
@mscdex
Copy link
Contributor

mscdex commented Mar 2, 2017

/cc @nodejs/crypto

@shigeki
Copy link
Contributor

shigeki commented Mar 2, 2017

I will take a look at it from now.

@shigeki
Copy link
Contributor

shigeki commented Mar 2, 2017

The reason is that instead of checking this preprocessor flag, node checks whether OPENSSL_NPN_NEGOTIATED is defined. However, the latter exists regardless to the former.

That's right. In ssl.h, #ifndef OPENSSL_NO_NEXTPROTONEG is used for NPN feature flag so that we have to change node_crypto.cc and others. I also found ALPN test is failed when NPN is disabled. I will submit a PR to fix this.

@shigeki
Copy link
Contributor

shigeki commented Mar 2, 2017

The fix is submitted in #11655

@shigeki shigeki closed this as completed in 02c98f4 Mar 3, 2017
shigeki added a commit that referenced this issue Mar 3, 2017
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]>
addaleax pushed a commit that referenced this issue Mar 5, 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.

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 issue Mar 5, 2017
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 pushed a commit that referenced this issue Apr 17, 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.

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 issue Apr 17, 2017
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 pushed a commit that referenced this issue Apr 19, 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.

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 issue Apr 19, 2017
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]>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 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.

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 issue Jul 19, 2017
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
Labels
build Issues and PRs related to build files or the CI. openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants