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

(v7.x backport) src: guard bundled_ca/openssl_ca with HAVE_OPENSSL #12662

Closed
wants to merge 2 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Apr 26, 2017

Currently, the following warning will be reported when configuring
without-ssl:

../src/node.cc:3653:8: warning: unused variable 'use_bundled_ca'
[-Wunused-variable]
bool use_bundled_ca = false;
^
../src/node.cc:3654:8: warning: unused variable 'use_openssl_ca'
[-Wunused-variable]
bool use_openssl_ca = false;
^

I missed this when working on
commit 8a7db9d ("src: add
--use-bundled-ca --use-openssl-ca check").

Refs: #12087
PR-URL: #12302
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: Richard Lau riclau@uk.ibm.com
Reviewed-By: Anna Henningsen anna@addaleax.net

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v7.x labels Apr 26, 2017
@mscdex mscdex added the openssl Issues and PRs related to the OpenSSL dependency. label Apr 26, 2017
src/node.cc Outdated
#if HAVE_OPENSSL
bool use_bundled_ca = false;
bool use_openssl_ca = false;
#endif // HAVE_INSPECTOR
Copy link
Member

Choose a reason for hiding this comment

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

Just noticed that the endif comment is off here. Also, this just adds the new variables… would they actually be necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thanks, I'll fix that (#12688).

And good point, I just assumed that the other PR had been merged, but it does not seem like it. I'll add it to this one and reorder them.

@evanlucas evanlucas force-pushed the v7.x-staging branch 2 times, most recently from d4ceb59 to 69a8053 Compare May 3, 2017 12:56
The --use-bundled-ca and --use-openssl-ca command line arguments are
mutually exclusive but can both be used on the same command line.

This commit adds a check if both options are used.

Fixes: nodejs#12083
PR-URL: nodejs#12087
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Currently, the following warning will be reported when configuring
without-ssl:

../src/node.cc:3653:8: warning: unused variable 'use_bundled_ca'
[-Wunused-variable]
  bool use_bundled_ca = false;
       ^
../src/node.cc:3654:8: warning: unused variable 'use_openssl_ca'
[-Wunused-variable]
  bool use_openssl_ca = false;
       ^

I missed this when working on
commit 8a7db9d ("src: add
--use-bundled-ca --use-openssl-ca check").

Refs: nodejs#12087
PR-URL: nodejs#12302
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@danbev
Copy link
Contributor Author

danbev commented May 30, 2017

@Trott
Copy link
Member

Trott commented Jul 15, 2017

Unfortunately, there are no more 7.x releases planned. Sorry this was never landed! I'm going to close it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants