★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/22392Nouvelle recherche | Portail wanayoo
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

src: refactor options parsing #22392

Closed
wants to merge 4 commits into from

Conversation

@addaleax
Copy link
Member

addaleax commented Aug 18, 2018

In case anybody was wondering what I’ve been up to this week. 😸


This is a major refactor of our Node’s parser. See node_options.cc
for how it is used, and node_options-inl.h for the bulk
of its implementation.

Unfortunately, the implementation has come to have some
complexity, in order to meet the following goals:

  • Make it easy to use for defining or changing options.
  • Keep it (mostly) backwards-compatible.
    • No tests were harmed as part of this commit.
  • Be as consistent as possible.
    • In particular, options can now generally accept arguments
      through both --foo=bar notation and --foo bar notation.
      We were previously very inconsistent on this point.
      (Labelling semver-minor because of this).
  • Separate into different levels of scope, namely
    per-process (global), per-Isolate and per-Environment
    (+ debug options).
  • Allow programmatic accessibility in the future.
    • This includes a possible expansion for --help output.

This commit also leaves a number of TODO comments, mostly for
improving consistency even more (possibly with having to modify
tests), improving embedder support, as well as removing pieces of
exposed configuration variables that should never have become
part of the public API but unfortunately are at this point.

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

Fyi @boneskull, you probably want to be aware of this because of #19335. :)

src: refactor options parsing
This is a major refactor of our Node’s parser. See `node_options.cc`
for how it is used, and  `node_options-inl.h` for the bulk
of its implementation.

Unfortunately, the implementation has come to have some
complexity, in order to meet the following goals:

- Make it easy to *use* for defining or changing options.
- Keep it (mostly) backwards-compatible.
  - No tests were harmed as part of this commit.
- Be as consistent as possible.
  - In particular, options can now generally accept arguments
    through both `--foo=bar` notation and `--foo bar` notation.
    We were previously very inconsistent on this point.
- Separate into different levels of scope, namely
  per-process (global), per-Isolate and per-Environment
  (+ debug options).
- Allow programmatic accessibility in the future.
  - This includes a possible expansion for `--help` output.

This commit also leaves a number of `TODO` comments, mostly for
improving consistency even more (possibly with having to modify
tests), improving embedder support, as well as removing pieces of
exposed configuration variables that should never have become
part of the public API but unfortunately are at this point.
@@ -72,35 +72,38 @@ static void Initialize(Local<Object> target,
READONLY_BOOLEAN_PROPERTY("hasTracing");
#endif

READONLY_STRING_PROPERTY(target, "icuDataDir", icu_data_dir);
// TODO(addaleax): This seems to be an unused, private API. Remove it?

This comment has been minimized.

@refack

refack Aug 18, 2018

Member

I remember @srl295 saying this might be used by embedders.

This comment has been minimized.

@addaleax

addaleax Aug 18, 2018

Author Member

It’s only accessible through process.binding('config'), so it’s still going to have to change somehow, assuming we push through with the process.binding() deprecations…

@refack
Copy link
Member

refack left a comment •

I love it! Way way way overdue refactoring.
But I don't like that OptionsParser is dynamically extensible. AFAIK it's a well established wheel, so either we assume an existing implementation, or make it static.

@refack refack dismissed their stale review Aug 18, 2018

Downgrading to comment

// These methods add a single option to the parser. Optionally, it can be
// specified whether the option should be allowed from environment variable
// sources (i.e. NODE_OPTIONS).
void AddOption(const std::string& name,

This comment has been minimized.

@refack

refack Aug 18, 2018

Member

Why can't these be templates?

This comment has been minimized.

@addaleax

addaleax Aug 18, 2018

Author Member

The way this is done is because we use it to infer the info.type value. We could do something with templates but I think it would mean we’d still use a list of template specializations?

@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Aug 18, 2018

But I don't like that OptionsParser is dynamically extensible. AFAIK it's a well established wheel, so either we assume an existing implementation, or make it static.

I don’t really understand this comment, can you explain or give examples?


namespace node {

DebugOptionsParser::DebugOptionsParser() {

This comment has been minimized.

@refack

refack Aug 18, 2018

Member

AFAIR this could be merged with EnvironmentOptionsParser. It was encapsulated as in preparation to something like this PR.

This comment has been minimized.

@addaleax

addaleax Aug 18, 2018

Author Member

Yes, this could be merged. As you are saying, it helps with encapsulation, so I don’t mind it either way.

This comment has been minimized.

@refack

refack Aug 18, 2018

Member

I would add a comment stating this, just so that future generations won't assume it's something special.

This comment has been minimized.

@addaleax

addaleax Aug 18, 2018

Author Member

Sounds good, done! (3c9b03a)

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Aug 18, 2018

Fantastic start on this. One the things I would really like to see is a quick way of determining if the options have changed from one run to another (essentially a hash value calculated from the options values)

@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Aug 18, 2018

@jasnell Just so I understand it, something that one could get today Hash(JSON.stringify(process.execArgv)) except that the values of all options are taken into account?

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Aug 18, 2018

Yes and no. Env vars also need to be taken into consideration and input should be normalized so that order doesn't matter (except in those handful of cases where it does, of course). It also needs to take into consideration default values that may happen to change.

@refack

This comment has been minimized.

Copy link
Member

refack commented Aug 18, 2018

But I don't like that OptionsParser is dynamically extensible. AFAIK it's a well established wheel, so either we assume an existing implementation, or make it static.

I don’t really understand this comment, can you explain or give examples?

I mean implementing a generic agrv parser. I don't have enough production experience with any specific library but https://github.com/search?l=C%2B%2B&q=argument+parser&type=Repositories show afew with > 100 stars.
If we don't want any of those I would have liked something static. So instead of:

AddOption("--inspect-port", &DebugOptions::host_port,
            kAllowedInEnvironment);
AddAlias("--debug-port", "--inspect-port");
AddOption("--inspect", &DebugOptions::inspector_enabled,
            kAllowedInEnvironment);
AddAlias("--inspect=", { "--inspect-port", "--inspect" });
AddOption("--debug", &DebugOptions::deprecated_debug);
AddAlias("--debug=", { "--inspect-port", "--debug" });

we would have something like

Option<"inspect-port", kAllowedInEnvironment> host_port;
Alias<"inspect-port"> debug-port;

that might requires reflection, or c'tors with side effects, or macros...

So in that sense your implementation is quite minimal, it's just that most of that information is known in compile time, so there no need to delegate it to run time.

@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Aug 19, 2018

I mean implementing a generic agrv parser. I don't have enough production experience with any specific library but https://github.com/search?l=C%2B%2B&q=argument+parser&type=Repositories show afew with > 100 stars.

I think that’s not doable given how weird some of our options are (think debug flags or --print/--eval), at least as long as we’re going for backwards compatibility…

So in that sense your implementation is quite minimal, it's just that most of that information is known in compile time, so there no need to delegate it to run time.

I agree that there is unnecessary runtime overhead here that I’d like to get down a bit, yes. 👍

bool Options::* field,
OptionEnvvarSettings env_setting) {
options_.emplace(name, OptionInfo {
kBoolean,

This comment has been minimized.

@refack

refack Aug 20, 2018

Member

Just thinking out loud... (It is an interesting problem after all).
How about instead on a const you put a pointer to the conversion function. That way you make these Templates specializations on the typeof(field), and eliminate the switch in L385.

This comment has been minimized.

@refack

refack Aug 20, 2018

Member

Had a few more "mind itches". I'll try to write up a draft.

This comment has been minimized.

@addaleax

addaleax Aug 20, 2018

Author Member

Had a few more "mind itches". I'll try to write up a draft.

Sounds good, looking forward to it!

@mcollina
Copy link
Member

mcollina left a comment

LGTM.

Can you also do a CITGM run?

@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Aug 20, 2018

@boneskull

This comment has been minimized.

Copy link
Contributor

boneskull commented Aug 20, 2018

@addaleax I don't think this conflicts too badly with #19335, but I have need to separate "options passed through to V8" (e.g., --abort-on-uncaught-exception) from "Node.js native options".

How can I use this API to get all flags which fall into either bucket?

@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Aug 20, 2018

@boneskull If this PR stays in its current shape, it shouldn’t be too hard – we could go through the options_ map of the parser, and take each entry where the type entry is kV8Option … that would currently yield --abort_on_uncaught_exception, --max_old_space_size, --perf_basic_prof, --perf_prof, --stack_trace_limit. Does that sound about right?

@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Aug 21, 2018

@refack Ping … anything you’d definitely like to see changed before merging this? It would be nice to get this in and work on expanding it (programmatic accessibility/help texts). :)

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

@refack

refack approved these changes Aug 21, 2018

@refack

This comment has been minimized.

Copy link
Member

refack commented Aug 21, 2018

anything you’d definitely like to see changed before merging this? It would be nice to get this in and work on expanding it (programmatic accessibility/help texts). :)

No reason to wait. If I find the time we could iterate later.

@refack

This comment has been minimized.

Copy link
Member

refack commented Aug 22, 2018 •

Why on macOS does gcc complains about std::atoll? We run it with -std=gnu++1y which I thought was >= c++14 > c++11
Ohh it's clang...

19:29:53 ../src/node_options-inl.h:392:33: error: no member named 'atoll' in namespace 'std'; did you mean simply 'atoll'?
19:29:53             ->Lookup(options) = std::atoll(value.c_str());
19:29:53                                 ^~~~~~~~~~
19:29:53                                 atoll
19:29:53 /usr/include/stdlib.h:136:3: note: 'atoll' declared here
19:29:53          atoll(const char *);
19:29:53          ^
19:29:53 ../src/node_options.cc:187:3: error: use of undeclared identifier 'errno'
19:29:53   errno = 0;
19:29:53   ^
19:29:53 ../src/node_options.cc:189:7: error: use of undeclared identifier 'errno'
19:29:53   if (errno != 0 || *endptr != '\0'||
19:29:53       ^
@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Aug 22, 2018

@refack I think it might just be that stdlib.h is included but cstdlib isn’t…

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


inline int ParseAndValidatePort(const std::string& port, std::string* error) {
char* endptr;
errno = 0;

This comment has been minimized.

@refack

refack Aug 22, 2018

Member

Isn't this lint? How did this compile before? Ahh the wonder of C++ compilers...
And in the next line could we switch to strtoul?

This comment has been minimized.

@addaleax

addaleax Aug 22, 2018

Author Member

Lint? It’s just a missing include, I assume… and, yes, I think we could switch, but it shouldn’t make any difference?

This comment has been minimized.

@refack

refack Aug 22, 2018

Member

It’s just a missing include

ahh it's defined in errno.h (that's a POSIX mechanism). Without that context it does look like an undefined var (who's value magicly changes).

@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Aug 22, 2018

Landed in 29a71ba 🎉

@addaleax addaleax closed this Aug 22, 2018

@addaleax addaleax deleted the addaleax:options branch Aug 22, 2018

addaleax added a commit that referenced this pull request Aug 22, 2018

src: refactor options parsing
This is a major refactor of our Node’s parser. See `node_options.cc`
for how it is used, and  `node_options-inl.h` for the bulk
of its implementation.

Unfortunately, the implementation has come to have some
complexity, in order to meet the following goals:

- Make it easy to *use* for defining or changing options.
- Keep it (mostly) backwards-compatible.
  - No tests were harmed as part of this commit.
- Be as consistent as possible.
  - In particular, options can now generally accept arguments
    through both `--foo=bar` notation and `--foo bar` notation.
    We were previously very inconsistent on this point.
- Separate into different levels of scope, namely
  per-process (global), per-Isolate and per-Environment
  (+ debug options).
- Allow programmatic accessibility in the future.
  - This includes a possible expansion for `--help` output.

This commit also leaves a number of `TODO` comments, mostly for
improving consistency even more (possibly with having to modify
tests), improving embedder support, as well as removing pieces of
exposed configuration variables that should never have become
part of the public API but unfortunately are at this point.

PR-URL: #22392
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
@refack

This comment has been minimized.

Copy link
Member

refack commented Aug 23, 2018

@addaleax PTAL at #22484. They found a hole in out CI matrix.

addaleax added a commit to addaleax/node that referenced this pull request Aug 26, 2018

src: fix NODE_OPTIONS parsing bug
I, uhm, might have messed up by using a `substr(start, end)`
signature when `std::string` actually uses `substr(start, len)`.
Fix that.

Fixes: nodejs#22526
Refs: nodejs#22392

@addaleax addaleax referenced this pull request Aug 26, 2018

Closed

src: fix NODE_OPTIONS parsing bug #22529

3 of 3 tasks complete

addaleax added a commit that referenced this pull request Aug 26, 2018

src: fix NODE_OPTIONS parsing bug
I, uhm, might have messed up by using a `substr(start, end)`
signature when `std::string` actually uses `substr(start, len)`.
Fix that.

Fixes: #22526
Refs: #22392

PR-URL: #22529
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

addaleax added a commit to addaleax/node that referenced this pull request Aug 27, 2018

src: refactor options parsing
This is a major refactor of our Node’s parser. See `node_options.cc`
for how it is used, and  `node_options-inl.h` for the bulk
of its implementation.

Unfortunately, the implementation has come to have some
complexity, in order to meet the following goals:

- Make it easy to *use* for defining or changing options.
- Keep it (mostly) backwards-compatible.
  - No tests were harmed as part of this commit.
- Be as consistent as possible.
  - In particular, options can now generally accept arguments
    through both `--foo=bar` notation and `--foo bar` notation.
    We were previously very inconsistent on this point.
- Separate into different levels of scope, namely
  per-process (global), per-Isolate and per-Environment
  (+ debug options).
- Allow programmatic accessibility in the future.
  - This includes a possible expansion for `--help` output.

This commit also leaves a number of `TODO` comments, mostly for
improving consistency even more (possibly with having to modify
tests), improving embedder support, as well as removing pieces of
exposed configuration variables that should never have become
part of the public API but unfortunately are at this point.

PR-URL: nodejs#22392
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>

addaleax added a commit that referenced this pull request Aug 28, 2018

src: refactor options parsing
This is a major refactor of our Node’s parser. See `node_options.cc`
for how it is used, and  `node_options-inl.h` for the bulk
of its implementation.

Unfortunately, the implementation has come to have some
complexity, in order to meet the following goals:

- Make it easy to *use* for defining or changing options.
- Keep it (mostly) backwards-compatible.
  - No tests were harmed as part of this commit.
- Be as consistent as possible.
  - In particular, options can now generally accept arguments
    through both `--foo=bar` notation and `--foo bar` notation.
    We were previously very inconsistent on this point.
- Separate into different levels of scope, namely
  per-process (global), per-Isolate and per-Environment
  (+ debug options).
- Allow programmatic accessibility in the future.
  - This includes a possible expansion for `--help` output.

This commit also leaves a number of `TODO` comments, mostly for
improving consistency even more (possibly with having to modify
tests), improving embedder support, as well as removing pieces of
exposed configuration variables that should never have become
part of the public API but unfortunately are at this point.

PR-URL: #22392
Backport-PR-URL: #22558
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

addaleax added a commit that referenced this pull request Aug 28, 2018

src: fix NODE_OPTIONS parsing bug
I, uhm, might have messed up by using a `substr(start, end)`
signature when `std::string` actually uses `substr(start, len)`.
Fix that.

Fixes: #22526
Refs: #22392

PR-URL: #22529
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

targos added a commit that referenced this pull request Sep 3, 2018

src: refactor options parsing
This is a major refactor of our Node’s parser. See `node_options.cc`
for how it is used, and  `node_options-inl.h` for the bulk
of its implementation.

Unfortunately, the implementation has come to have some
complexity, in order to meet the following goals:

- Make it easy to *use* for defining or changing options.
- Keep it (mostly) backwards-compatible.
  - No tests were harmed as part of this commit.
- Be as consistent as possible.
  - In particular, options can now generally accept arguments
    through both `--foo=bar` notation and `--foo bar` notation.
    We were previously very inconsistent on this point.
- Separate into different levels of scope, namely
  per-process (global), per-Isolate and per-Environment
  (+ debug options).
- Allow programmatic accessibility in the future.
  - This includes a possible expansion for `--help` output.

This commit also leaves a number of `TODO` comments, mostly for
improving consistency even more (possibly with having to modify
tests), improving embedder support, as well as removing pieces of
exposed configuration variables that should never have become
part of the public API but unfortunately are at this point.

PR-URL: #22392
Backport-PR-URL: #22558
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

targos added a commit that referenced this pull request Sep 3, 2018

src: fix NODE_OPTIONS parsing bug
I, uhm, might have messed up by using a `substr(start, end)`
signature when `std::string` actually uses `substr(start, len)`.
Fix that.

Fixes: #22526
Refs: #22392

PR-URL: #22529
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

targos added a commit that referenced this pull request Sep 5, 2018

2018-09-06, Version 10.10.0 (Current)
Notable changes:

* child_process:
  * `TypedArray` and `DataView` values are now accepted as input by
    `execFileSync` and `spawnSync`. #22409
* coverage:
  * Native V8 code coverage information can now be output to disk by setting the
    environment variable `NODE_V8_COVERAGE` to a directory. #22527
* deps:
  * The bundled npm was upgraded to version 6.4.1. #22591
    * Changelogs:
      [6.3.0-next.0](https://github.com/npm/cli/releases/tag/v6.3.0-next.0)
      [6.3.0](https://github.com/npm/cli/releases/tag/v6.3.0)
      [6.4.0](https://github.com/npm/cli/releases/tag/v6.4.0)
      [6.4.1](https://github.com/npm/cli/releases/tag/v6.4.1)
* fs:
  * The methods `fs.read`, `fs.readSync`, `fs.write`, `fs.writeSync`,
    `fs.writeFile` and `fs.writeFileSync` now all accept `TypedArray` and
    `DataView` objects. #22150
  * A new boolean option, `withFileTypes`, can be passed to to `fs.readdir` and
    `fs.readdirSync`. If set to true, the methods return an array of directory
    entries. These are objects that can be used to determine the type of each
    entry and filter them based on that without calling `fs.stat`. #22020
* http2:
  * The `http2` module is no longer experimental. #22466
* os:
  * Added two new methods: `os.getPriority` and `os.setPriority`, allowing to
    manipulate the scheduling priority of processes. #22394
* process:
  * Added `process.allowedNodeEnvironmentFlags`. This object can be used to
    programmatically validate and list flags that are allowed in the
    `NODE_OPTIONS` environment variable. #19335
* src:
  * Deprecated option variables in public C++ API. #22392
  * Refactored options parsing. #22392
* vm:
  * Added `vm.compileFunction`, a method to create new JavaScript functions from
    a source body, with options similar to those of the other `vm` methods. #21571
* Added new collaborators:
  * [lundibundi](https://github.com/lundibundi) - Denys Otrishko

targos added a commit that referenced this pull request Sep 5, 2018

2018-09-06, Version 10.10.0 (Current)
Notable changes:

* child_process:
  * `TypedArray` and `DataView` values are now accepted as input by
    `execFileSync` and `spawnSync`. #22409
* coverage:
  * Native V8 code coverage information can now be output to disk by setting the
    environment variable `NODE_V8_COVERAGE` to a directory. #22527
* deps:
  * The bundled npm was upgraded to version 6.4.1. #22591
    * Changelogs:
      [6.3.0-next.0](https://github.com/npm/cli/releases/tag/v6.3.0-next.0)
      [6.3.0](https://github.com/npm/cli/releases/tag/v6.3.0)
      [6.4.0](https://github.com/npm/cli/releases/tag/v6.4.0)
      [6.4.1](https://github.com/npm/cli/releases/tag/v6.4.1)
* fs:
  * The methods `fs.read`, `fs.readSync`, `fs.write`, `fs.writeSync`,
    `fs.writeFile` and `fs.writeFileSync` now all accept `TypedArray` and
    `DataView` objects. #22150
  * A new boolean option, `withFileTypes`, can be passed to to `fs.readdir` and
    `fs.readdirSync`. If set to true, the methods return an array of directory
    entries. These are objects that can be used to determine the type of each
    entry and filter them based on that without calling `fs.stat`. #22020
* http2:
  * The `http2` module is no longer experimental. #22466
* os:
  * Added two new methods: `os.getPriority` and `os.setPriority`, allowing to
    manipulate the scheduling priority of processes. #22394
* process:
  * Added `process.allowedNodeEnvironmentFlags`. This object can be used to
    programmatically validate and list flags that are allowed in the
    `NODE_OPTIONS` environment variable. #19335
* src:
  * Deprecated option variables in public C++ API. #22392
  * Refactored options parsing. #22392
* vm:
  * Added `vm.compileFunction`, a method to create new JavaScript functions from
    a source body, with options similar to those of the other `vm` methods. #21571
* Added new collaborators:
  * [lundibundi](https://github.com/lundibundi) - Denys Otrishko

PR-URL: #22716

@targos targos referenced this pull request Sep 5, 2018

Merged

Release proposal: v10.10.0 #22716

targos added a commit that referenced this pull request Sep 6, 2018

src: refactor options parsing
This is a major refactor of our Node’s parser. See `node_options.cc`
for how it is used, and  `node_options-inl.h` for the bulk
of its implementation.

Unfortunately, the implementation has come to have some
complexity, in order to meet the following goals:

- Make it easy to *use* for defining or changing options.
- Keep it (mostly) backwards-compatible.
  - No tests were harmed as part of this commit.
- Be as consistent as possible.
  - In particular, options can now generally accept arguments
    through both `--foo=bar` notation and `--foo bar` notation.
    We were previously very inconsistent on this point.
- Separate into different levels of scope, namely
  per-process (global), per-Isolate and per-Environment
  (+ debug options).
- Allow programmatic accessibility in the future.
  - This includes a possible expansion for `--help` output.

This commit also leaves a number of `TODO` comments, mostly for
improving consistency even more (possibly with having to modify
tests), improving embedder support, as well as removing pieces of
exposed configuration variables that should never have become
part of the public API but unfortunately are at this point.

PR-URL: #22392
Backport-PR-URL: #22558
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

targos added a commit that referenced this pull request Sep 6, 2018

src: fix NODE_OPTIONS parsing bug
I, uhm, might have messed up by using a `substr(start, end)`
signature when `std::string` actually uses `substr(start, len)`.
Fix that.

Fixes: #22526
Refs: #22392

PR-URL: #22529
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

targos added a commit that referenced this pull request Sep 6, 2018

2018-09-06, Version 10.10.0 (Current)
Notable changes:

* child_process:
  * `TypedArray` and `DataView` values are now accepted as input by
    `execFileSync` and `spawnSync`. #22409
* coverage:
  * Native V8 code coverage information can now be output to disk by setting the
    environment variable `NODE_V8_COVERAGE` to a directory. #22527
* deps:
  * The bundled npm was upgraded to version 6.4.1. #22591
    * Changelogs:
      [6.3.0-next.0](https://github.com/npm/cli/releases/tag/v6.3.0-next.0)
      [6.3.0](https://github.com/npm/cli/releases/tag/v6.3.0)
      [6.4.0](https://github.com/npm/cli/releases/tag/v6.4.0)
      [6.4.1](https://github.com/npm/cli/releases/tag/v6.4.1)
* fs:
  * The methods `fs.read`, `fs.readSync`, `fs.write`, `fs.writeSync`,
    `fs.writeFile` and `fs.writeFileSync` now all accept `TypedArray` and
    `DataView` objects. #22150
  * A new boolean option, `withFileTypes`, can be passed to to `fs.readdir` and
    `fs.readdirSync`. If set to true, the methods return an array of directory
    entries. These are objects that can be used to determine the type of each
    entry and filter them based on that without calling `fs.stat`. #22020
* http2:
  * The `http2` module is no longer experimental. #22466
* os:
  * Added two new methods: `os.getPriority` and `os.setPriority`, allowing to
    manipulate the scheduling priority of processes. #22407
* process:
  * Added `process.allowedNodeEnvironmentFlags`. This object can be used to
    programmatically validate and list flags that are allowed in the
    `NODE_OPTIONS` environment variable. #19335
* src:
  * Deprecated option variables in public C++ API. #22515
  * Refactored options parsing. #22392
* vm:
  * Added `vm.compileFunction`, a method to create new JavaScript functions from
    a source body, with options similar to those of the other `vm` methods. #21571
* Added new collaborators:
  * [lundibundi](https://github.com/lundibundi) - Denys Otrishko

PR-URL: #22716

targos added a commit that referenced this pull request Sep 6, 2018

2018-09-06, Version 10.10.0 (Current)
Notable changes:

* child_process:
  * `TypedArray` and `DataView` values are now accepted as input by
    `execFileSync` and `spawnSync`. #22409
* coverage:
  * Native V8 code coverage information can now be output to disk by setting the
    environment variable `NODE_V8_COVERAGE` to a directory. #22527
* deps:
  * The bundled npm was upgraded to version 6.4.1. #22591
    * Changelogs:
      [6.3.0-next.0](https://github.com/npm/cli/releases/tag/v6.3.0-next.0)
      [6.3.0](https://github.com/npm/cli/releases/tag/v6.3.0)
      [6.4.0](https://github.com/npm/cli/releases/tag/v6.4.0)
      [6.4.1](https://github.com/npm/cli/releases/tag/v6.4.1)
* fs:
  * The methods `fs.read`, `fs.readSync`, `fs.write`, `fs.writeSync`,
    `fs.writeFile` and `fs.writeFileSync` now all accept `TypedArray` and
    `DataView` objects. #22150
  * A new boolean option, `withFileTypes`, can be passed to to `fs.readdir` and
    `fs.readdirSync`. If set to true, the methods return an array of directory
    entries. These are objects that can be used to determine the type of each
    entry and filter them based on that without calling `fs.stat`. #22020
* http2:
  * The `http2` module is no longer experimental. #22466
* os:
  * Added two new methods: `os.getPriority` and `os.setPriority`, allowing to
    manipulate the scheduling priority of processes. #22407
* process:
  * Added `process.allowedNodeEnvironmentFlags`. This object can be used to
    programmatically validate and list flags that are allowed in the
    `NODE_OPTIONS` environment variable. #19335
* src:
  * Deprecated option variables in public C++ API. #22515
  * Refactored options parsing. #22392
* vm:
  * Added `vm.compileFunction`, a method to create new JavaScript functions from
    a source body, with options similar to those of the other `vm` methods. #21571
* Added new collaborators:
  * [lundibundi](https://github.com/lundibundi) - Denys Otrishko

PR-URL: #22716

targos added a commit that referenced this pull request Sep 6, 2018

2018-09-06, Version 10.10.0 (Current)
Notable changes:

* child_process:
  * `TypedArray` and `DataView` values are now accepted as input by
    `execFileSync` and `spawnSync`. #22409
* coverage:
  * Native V8 code coverage information can now be output to disk by setting the
    environment variable `NODE_V8_COVERAGE` to a directory. #22527
* deps:
  * The bundled npm was upgraded to version 6.4.1. #22591
    * Changelogs:
      [6.3.0-next.0](https://github.com/npm/cli/releases/tag/v6.3.0-next.0)
      [6.3.0](https://github.com/npm/cli/releases/tag/v6.3.0)
      [6.4.0](https://github.com/npm/cli/releases/tag/v6.4.0)
      [6.4.1](https://github.com/npm/cli/releases/tag/v6.4.1)
* fs:
  * The methods `fs.read`, `fs.readSync`, `fs.write`, `fs.writeSync`,
    `fs.writeFile` and `fs.writeFileSync` now all accept `TypedArray` and
    `DataView` objects. #22150
  * A new boolean option, `withFileTypes`, can be passed to to `fs.readdir` and
    `fs.readdirSync`. If set to true, the methods return an array of directory
    entries. These are objects that can be used to determine the type of each
    entry and filter them based on that without calling `fs.stat`. #22020
* http2:
  * The `http2` module is no longer experimental. #22466
* os:
  * Added two new methods: `os.getPriority` and `os.setPriority`, allowing to
    manipulate the scheduling priority of processes. #22407
* process:
  * Added `process.allowedNodeEnvironmentFlags`. This object can be used to
    programmatically validate and list flags that are allowed in the
    `NODE_OPTIONS` environment variable. #19335
* src:
  * Deprecated option variables in public C++ API. #22515
  * Refactored options parsing. #22392
* vm:
  * Added `vm.compileFunction`, a method to create new JavaScript functions from
    a source body, with options similar to those of the other `vm` methods. #21571
* Added new collaborators:
  * [lundibundi](https://github.com/lundibundi) - Denys Otrishko

PR-URL: #22716

@targos targos added this to Backported in v10.x Sep 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.