GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — src: add --use-bundled-ca --use-openssl-ca check by danbev · Pull Request #12087 · nodejs/node

Conversation

danbev commented Mar 28, 2017

Copy link

Copy Markdown

Contributor

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

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

src

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

nodejs-github-bot added build

Issues and PRs related to Node.js builds or CI infrastructure.

c++ Issues and PRs that require attention from people who are familiar with C++. labels

Mar 28, 2017

danbev commented Mar 28, 2017

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

danbev commented Mar 28, 2017

Copy link

Copy Markdown

Contributor Author

test/osx/ failure looks unrelated:

not ok 1411 sequential/test-fs-readfile-tostring-fail
  ---
  duration_ms: 60.609
  severity: fail
  stack: |-
    timeout
  ...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mscdex added openssl

Issues and PRs related to the OpenSSL dependency.

cli Issues and PRs related to the Node.js command-line interface. and removed build Issues and PRs related to Node.js builds or CI infrastructure. labels

Mar 28, 2017

bnoordhuis approved these changes

Mar 28, 2017

View reviewed changes

bnoordhuis left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM modulo nits. It's nice we can have cctests for such things now.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/node.cc Outdated

bnoordhuis Mar 28, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you line up the arguments? There should probably be an "either" in front of the error message and we prefix it with argv[0] in most cases.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

danbev Mar 28, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ah I see that now, I'll fix that. Thx

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bnoordhuis approved these changes

Mar 28, 2017

View reviewed changes

richardlau commented Mar 28, 2017

Copy link

Copy Markdown

Member

Are there any pros/cons for having the test written as a cctest as opposed to JavaScript?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

danbev commented Mar 28, 2017

Copy link

Copy Markdown

Contributor Author

@richardlau There are some situations where it might be easier to write unit tests, one example would be like the above when need to test command line options to Node. Another that we came across was when testing a feature that was intended to be used when Node is embedded (for example Electron). At least that is my take on it. There is a section in the writing-tests.md which mentions this.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

richardlau commented Mar 28, 2017

Copy link

Copy Markdown

Member

For testing embedded Node.js cctest makes sense. My concerns for things like the test in this PR is:

If you ever want to tweak/extend it you have to recompile the cctests.
The portion of the community who would be willing to look at the test if it was written in JavaScript compared to if the test was written in C++.

cc @nodejs/testing

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cjihrig requested changes

Mar 28, 2017

View reviewed changes

cjihrig left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think this PR would be simpler, and more people would maintain this code in the future if the test were written in JS.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

danbev commented Mar 28, 2017

Copy link

Copy Markdown

Contributor Author

I actually thought using a C++ unit test in this case was making it simpler. The motivation for that is that it is testing command line arguments that are to be passed to Node upon startup. Maybe it is my lack of experience, but I can't think of a good way of testing that from JavaScript.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gibfahn commented Mar 28, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

@danbev couldn't you just do something like this?

const execSync = require('child_process').execSync;
const output = execSync(`${process.execPath} --use-bundled-ca --use-openssl-ca`);
assert.strictEqual(output.toString(), "Error message...");

cjihrig reacted with laugh emoji

All reactions

😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cjihrig commented Mar 28, 2017

Copy link

Copy Markdown

Contributor

I think the simplest way would be to use child_process.spawnSync(process.execPath, ['--use-bundled-ca', '--use-openssl-ca']).

gibfahn and richardlau reacted with thumbs up emoji

All reactions

👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gibfahn commented Mar 28, 2017

Copy link

Copy Markdown

Member

@cjihrig I recall that there is a reason to use one of spawnSync and execSync over the other, possibly to do with things working as expected on Windows. I can't remember which one it was though.

Is spawnSync the preferred one?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cjihrig commented Mar 28, 2017

Copy link

Copy Markdown

Contributor

exec() runs a command to completion in a shell and then calls back with the result. execSync() is a blocking version of that. execFile() is the same, but without the shell.

spawn() is the most flexible, but usually requires the most effort/code to control. Under the hood, spawn() is used by the other asynchronous child process methods. spawnSync() takes the work out of using spawn(), if you can afford to block (which we can in a test like this). We don't need the shell in this case, so I'd go with spawnSync().

gibfahn reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

danbev commented Mar 28, 2017

Copy link

Copy Markdown

Contributor Author

@gibfahn @cjihrig Thanks for that, I'll give that a go and see what people think.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

richardlau reviewed

Mar 28, 2017

View reviewed changes

Comment thread

test/parallel/test-openssl-ca-options.js Outdated

richardlau Mar 28, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You can avoid having to call toString() here if you pass an encoding (e.g. utf8) option to child_process.spawnSync.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

danbev Mar 28, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ah nice, I'll do that. Thx

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

danbev commented Mar 28, 2017

Copy link

Copy Markdown

Contributor Author

I've added a suggestion for the JavaScript test based on the suggestions from @gibfahn and @cjihrig.

My personal opinion is that the C++ unit test is just as readable/simple but I'm probably a little bias in this case :) It seems like the majority if for having this in JavaScript and I'll update the PR accordingly. Will let a few others chime and see if what the outcome is.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cjihrig commented Mar 28, 2017

Copy link

Copy Markdown

Contributor

My personal opinion is that the C++ unit test is just as readable/simple but I'm probably a little bias in this case

I think the test itself is about equal in complexity, but it no longer requires changes to the gyp file and the fixture file. It also makes the test more accessible to a larger number of contributors.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cjihrig reviewed

Mar 28, 2017

View reviewed changes

Comment thread

test/parallel/test-openssl-ca-options.js Outdated

cjihrig Mar 28, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

After this line, can you add:

if (!common.hasCrypto) {
  common.skip('missing crypto');
  process.exit();
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

danbev Mar 28, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ah yes, I'll add that. Thx

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

danbev commented Mar 28, 2017

Copy link

Copy Markdown

Contributor Author

I think the test itself is about equal in complexity, but it no longer requires changes to the gyp file and the fixture file.

The change to the fixture would not normally be part of that. This was a bug that I introduced but did not discover until I actually used the arguments in this PR :( But there would still be a change required for the gyp file.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gibfahn reviewed

Mar 28, 2017

View reviewed changes

Comment thread

test/parallel/test-openssl-ca-options.js Outdated

gibfahn Mar 28, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

nit check -> checks

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

danbev Mar 28, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed now, thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gibfahn reacted with thumbs up emoji

All reactions

👍 1 reaction

danbev added a commit to danbev/node that referenced this pull request

Mar 29, 2017


          test: fix truncation of argv

32d9f3f

Currently argv_[1] and argv_[2] are getting truncated by one character
because of an incorrect addition of one to account for the null
character. I only noticed this when working on nodejs#12087, but that fix
will probably not get included in favor of a JavaScript test so I'm
adding this separate commit for it.

Refs: nodejs#12087

danbev mentioned this pull request

Mar 29, 2017

test: fix truncation of argv #12110

Closed

2 tasks done

gibfahn commented Mar 29, 2017

Copy link

Copy Markdown

Member

I think the test itself is about equal in complexity, but it no longer requires changes to the gyp file and the fixture file. It also makes the test more accessible to a larger number of contributors.

For me the accessibility is key, I think the vast majority of node collaborators are more familiar with tests written in js, not least for things like using the inspector for debugging.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gibfahn approved these changes

Mar 29, 2017

View reviewed changes

danbev mentioned this pull request

Jan 12, 2018

[v6.x backport] test: fix truncation of argv #18113

Closed

2 tasks done

MylesBorins pushed a commit that referenced this pull request

Jan 17, 2018


          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


          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 pushed a commit that referenced this pull request

Jan 17, 2018


          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


          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

Jan 18, 2018


          test: fix truncation of argv

f374d60

Currently argv_[1] and argv_[2] are getting truncated by one character
because of an incorrect addition of one to account for the null
character. I only noticed this when working on #12087, but that fix
will probably not get included in favor of a JavaScript test so I'm
adding this separate commit for it.

Refs: #12087
Backport-PR-URL: #18113
PR-URL: #12110
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>

MylesBorins mentioned this pull request

Jan 24, 2018

v6.13.0 proposal #18342

Merged

MylesBorins pushed a commit that referenced this pull request

Feb 11, 2018


          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


          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 11, 2018


          test: fix truncation of argv

3c6e327

Currently argv_[1] and argv_[2] are getting truncated by one character
because of an incorrect addition of one to account for the null
character. I only noticed this when working on #12087, but that fix
will probably not get included in favor of a JavaScript test so I'm
adding this separate commit for it.

Refs: #12087
Backport-PR-URL: #18113
PR-URL: #12110
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>

MylesBorins added a commit that referenced this pull request

Feb 11, 2018


          2018-01-13, Version 6.13.0 'Boron' (LTS)

b5bef9c

This LTS release comes with 109 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 29 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    #12678
* crypto:
  - expose ECDH class (Bryan English)
    #8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    #10209
  - warn on invalid authentication tag length (Tobias Nießen)
    #17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    #16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    #7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    #13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    #13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    #16386
* net:
  - return this from getConnections() (Sam Roberts)
    #13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    #13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    #14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    #16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    #12087
  - add process.ppid (cjihrig)
    #16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    #12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    #15179
* url:
  - WHATWG URL api support (James M Snell)
    #7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    #10308

PR-URL: #18342

MylesBorins pushed a commit that referenced this pull request

Feb 12, 2018


          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


          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 12, 2018


          test: fix truncation of argv

7510b97

Currently argv_[1] and argv_[2] are getting truncated by one character
because of an incorrect addition of one to account for the null
character. I only noticed this when working on #12087, but that fix
will probably not get included in favor of a JavaScript test so I'm
adding this separate commit for it.

Refs: #12087
Backport-PR-URL: #18113
PR-URL: #12110
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>

MylesBorins added a commit that referenced this pull request

Feb 12, 2018


          2018-01-13, Version 6.13.0 'Boron' (LTS)

8317be1

This LTS release comes with 109 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 29 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    #12678
* crypto:
  - expose ECDH class (Bryan English)
    #8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    #10209
  - warn on invalid authentication tag length (Tobias Nießen)
    #17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    #16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    #7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    #13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    #13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    #16386
* net:
  - return this from getConnections() (Sam Roberts)
    #13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    #13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    #14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    #16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    #12087
  - add process.ppid (cjihrig)
    #16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    #12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    #15179
* url:
  - WHATWG URL api support (James M Snell)
    #7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    #10308

PR-URL: #18342

MylesBorins pushed a commit that referenced this pull request

Feb 13, 2018


          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


          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]>

MylesBorins pushed a commit that referenced this pull request

Feb 13, 2018


          test: fix truncation of argv

5f79448

Currently argv_[1] and argv_[2] are getting truncated by one character
because of an incorrect addition of one to account for the null
character. I only noticed this when working on #12087, but that fix
will probably not get included in favor of a JavaScript test so I'm
adding this separate commit for it.

Refs: #12087
Backport-PR-URL: #18113
PR-URL: #12110
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>

MylesBorins added a commit that referenced this pull request

Feb 13, 2018


          2018-01-13, Version 6.13.0 'Boron' (LTS)

0f7dcba

This LTS release comes with 109 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 29 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    #12678
* crypto:
  - expose ECDH class (Bryan English)
    #8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    #10209
  - warn on invalid authentication tag length (Tobias Nießen)
    #17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    #16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    #7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    #13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    #13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    #16386
* net:
  - return this from getConnections() (Sam Roberts)
    #13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    #13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    #14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    #16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    #12087
  - add process.ppid (cjihrig)
    #16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    #12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    #15179
* url:
  - WHATWG URL api support (James M Snell)
    #7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    #10308

PR-URL: #18342

MylesBorins added a commit that referenced this pull request

Feb 13, 2018


          2018-01-13, Version 6.13.0 'Boron' (LTS)

This LTS release comes with 112 commits, 17 of which are considered
Semver-Minor. This includes 32 which are doc related, 30 which are test
related, 8 which are build / tool related and 1 commit which updates
a dependency.

Notable Changes:

* console:
  - added console.count() and console.clear() (James M Snell)
    #12678
* crypto:
  - expose ECDH class (Bryan English)
    #8188
  - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas)
    #10209
  - warn on invalid authentication tag length (Tobias Nießen)
    #17566
* deps:
  - upgrade libuv to 1.16.1 (cjihrig)
    #16835
* dgram:
  - added socket.setMulticastInterface() (Will Young)
    #7855
* http:
  - add agent.keepSocketAlive and agent.reuseSocket as to allow
    overridable keep-alive behavior of `Agent` (Fedor Indutny)
    #13005
* lib:
  - return this from net.Socket.end() (Sam Roberts)
    #13481
* module:
  - add builtinModules api that provides list of all builtin modules in
    Node (Jon Moss)
    #16386
* net:
  - return this from getConnections() (Sam Roberts)
    #13553
* promises:
  - more robust stringification for unhandled rejections (Timothy Gu)
    #13784
* repl:
  - improve require() autocompletion (Alexey Orlenko)
    #14409
* src:
  - add openssl-system-ca-path configure option (Daniel Bevenius)
    #16790
  - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius)
    #12087
  - add process.ppid (cjihrig)
    #16839
* tls:
  - accept `lookup` option for `tls.connect()` (Fedor Indutny)
    #12839
* tools, build:
  - a new macOS installer! (JP Wesselink)
    #15179
* url:
  - WHATWG URL api support (James M Snell)
    #7448
* util:
  - add %i and %f formatting specifiers (Roman Reiss)
    #10308

PR-URL: #18342

Reviewers

jasnell

jasnell left review comments

bnoordhuis

bnoordhuis approved these changes

cjihrig

cjihrig approved these changes

richardlau

richardlau approved these changes

+2 more reviewers

gibfahn

gibfahn approved these changes

sam-github

sam-github approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++

Issues and PRs that require attention from people who are familiar with C++.

cli

Issues and PRs related to the Node.js command-line interface.

openssl

Issues and PRs related to the OpenSSL dependency.

semver-major

PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

--use-bundled-ca and --use-openssl-ca order

Uh oh!

There was an error while loading. Please reload this page.

9 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

danbev commented Mar 28, 2017

Checklist

Affected core subsystem(s)

Uh oh!

danbev commented Mar 28, 2017

Uh oh!

danbev commented Mar 28, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richardlau commented Mar 28, 2017

Uh oh!

danbev commented Mar 28, 2017

Uh oh!

richardlau commented Mar 28, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

danbev commented Mar 28, 2017

Uh oh!

gibfahn commented Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig commented Mar 28, 2017

Uh oh!

gibfahn commented Mar 28, 2017

Uh oh!

cjihrig commented Mar 28, 2017

Uh oh!

danbev commented Mar 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbev commented Mar 28, 2017

Uh oh!

cjihrig commented Mar 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbev commented Mar 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibfahn commented Mar 29, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

gibfahn commented Mar 28, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.