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

[v6.x backport] src: guard bundled_ca/openssl_ca with HAVE_OPENSSL #17783

Closed
wants to merge 2 commits into from
Closed

[v6.x backport] src: guard bundled_ca/openssl_ca with HAVE_OPENSSL #17783

wants to merge 2 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Dec 20, 2017

This is a backport of the following PRs:

Please note that there was a previous PR for backporting this, but was closed as noted in this comment.

@MylesBorins I'm not sure if this should land due to your comment above. Could you take a look and let me know what you think?

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

@danbev
src: add --use-bundled-ca --use-openssl-ca check
c3f35be 
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 <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@danbev
src: guard bundled_ca/openssl_ca with HAVE_OPENSSL
c7bea9f 
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 <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v6.x labels Dec 20, 2017
@danbev
Copy link
Contributor Author

danbev commented Dec 20, 2017

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

@MylesBorins MylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 20, 2017
@MylesBorins
Copy link
Contributor

thanks for opening @danbev

my apologies if we have already reviewed this before. I'm defensively marking as semver minor, and we can discuss with the @nodejs/lts team

@MylesBorins
Copy link
Contributor

Should potentially come with #12688 as well

@MylesBorins
Copy link
Contributor

landed in 5f6f460...64f0967

MylesBorins pushed a commit that referenced this pull request Jan 17, 2018
@danbev @MylesBorins
src: add --use-bundled-ca --use-openssl-ca check
0d08e03 
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: #12083
Backport-PR-URL: #17783
PR-URL: #12087
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 17, 2018
@danbev @MylesBorins
src: guard bundled_ca/openssl_ca with HAVE_OPENSSL
64f0967 
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
Backport-PR-URL: #17783
PR-URL: #12302
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins reopened this Jan 17, 2018
MylesBorins pushed a commit that referenced this pull request Jan 17, 2018
@danbev @MylesBorins
src: add --use-bundled-ca --use-openssl-ca check
dd2102d 
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: #12083
Backport-PR-URL: #17783
PR-URL: #12087
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 17, 2018
@danbev @MylesBorins
src: guard bundled_ca/openssl_ca with HAVE_OPENSSL
c3d5fb3 
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
Backport-PR-URL: #17783
PR-URL: #12302
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
@danbev @MylesBorins
src: add --use-bundled-ca --use-openssl-ca check
9ab8467 
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: #12083
Backport-PR-URL: #17783
PR-URL: #12087
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
@danbev @MylesBorins
src: guard bundled_ca/openssl_ca with HAVE_OPENSSL
1b38147 
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
Backport-PR-URL: #17783
PR-URL: #12302
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
@danbev @MylesBorins
src: add --use-bundled-ca --use-openssl-ca check
6d291ef 
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: #12083
Backport-PR-URL: #17783
PR-URL: #12087
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
@danbev @MylesBorins
src: guard bundled_ca/openssl_ca with HAVE_OPENSSL
0e0a783 
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
Backport-PR-URL: #17783
PR-URL: #12302
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
@danbev @MylesBorins
src: add --use-bundled-ca --use-openssl-ca check
758dc81 
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: #12083
Backport-PR-URL: #17783
PR-URL: #12087
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
@danbev @MylesBorins
src: guard bundled_ca/openssl_ca with HAVE_OPENSSL
2b29cea 
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
Backport-PR-URL: #17783
PR-URL: #12302
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@danbev danbev deleted the backport-12302-v6.x branch March 27, 2018 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@MylesBorins MylesBorins

Labels
c++ Issues and PRs that require attention from people who are familiar with C++. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@danbev @MylesBorins @nodejs-github-bot