doc, src: sort cli cmd options and env vars #19878

willhayslett · 2018-04-08T06:56:51Z

Alphabetizing the command line options and environment
variables in doc/api/cli.md for consistency and readability.

Refs: #19814

Checklist

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

vsemozhetbyt · 2018-04-08T09:52:56Z

Tricky diff)

vsemozhetbyt · 2018-04-08T10:25:56Z

Let's do the check in some steps.

Sort all lines in temporal copies of both versions, compare the result and make sure nothing unwanted is added/deleted.

Found now during the comparison:

+ 14 blank lines: more consistent space between sections, this is good.
These changes in metadata seem accidental?

### `--force-fips`
<!-- YAML
- added: v6.0.0
 -->

### `--force-fips`
<!-- YAML
+added:
+  - v5.0.0
+  - v4.2.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/19600
+    description: The `--require` option is now supported when checking a file.
 -->

### `-c`, `--check`
<!-- YAML
added:
  - v5.0.0
  - v4.2.0
-changes:
-  - version: REPLACEME
-    pr-url: https://github.com/nodejs/node/pull/19600
-    description: The `--require` option is now supported when checking a file.
 -->

### `-c`, `--check`
<!-- YAML
added:
  - v5.0.0
  - v4.2.0
 -->

vsemozhetbyt · 2018-04-08T10:29:59Z

While fixing the metadata change, you can also add an extra blank line between ### `NODE_NO_WARNINGS=1` and ### `NODE_OPTIONS=options...` sections for consistensy.

willhayslett · 2018-04-08T16:11:00Z

Yes, very tricky diff; thank you for the thorough review!

I got a little tripped up on the force-fips metadata changes. Encountered a conflict with the following sha while rebasing:

0876a03#diff-85b6573bc4afd5e06ed3b9d9b553635d

~~Looks like the force-fips change is correct, but the removal from --check isn't.~~

EDIT: Looks like the metadata between those options got switched.

I'll go ahead get this updated along with the blank lines, but let me know if that doesn't seem correct.

vsemozhetbyt · 2018-04-08T17:15:49Z

It seems the removal from --check isn't correct, but I doubt if --force-fips change is correct: the changed and added metadata seems just copied from the --check change. Maybe --force-fips change should be reverted and --check metadata updated (during rebase or in a manual way)?

willhayslett · 2018-04-08T17:52:40Z

@vsemozhetbyt, you're right. Making these changes now.

vsemozhetbyt · 2018-04-08T18:28:42Z

Sort all lines in temporal copies of both versions, compare the result and make sure nothing unwanted is added/deleted. — Check!

Now I am trying to find a way to check if nothing is changed inside the reordered sections by chance...

willhayslett · 2018-04-08T18:39:24Z

@vsemozhetbyt it's definitely non-trivial to sift through the changes.

I'm also working on making the same adjustments in node.cc so the options/vars are sorted when executing node --help.

If there happens to be a better way to do it so the diff isn't so difficult to read, please let me know.

vsemozhetbyt · 2018-04-08T18:58:40Z

I am not aware of any simpler way(

But to check if the sections are not changed I've used this trick

Replace in both versions /\n(?!#|\[.+\]:)/ with ' ' to make all the sections one-liners (except bottom references).
Trim trailing spaces.
Sort all lines.
The files should be equals.

And they are!

vsemozhetbyt · 2018-04-08T19:16:44Z

Sorting is also OK.

vsemozhetbyt

For now:

Nothing is added or deleted except appropriate new lines.
Nothing is regrouped inside the sections.
Sorting is right.

vsemozhetbyt · 2018-04-08T19:29:58Z

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

vsemozhetbyt · 2018-04-08T19:33:09Z

Changes in node.cc can be added to this PR as a separate commit.

willhayslett · 2018-04-08T22:28:41Z

@vsemozhetbyt, as there were some logic changes with the latest commit, I'd like to take a stab at creating some c++ tests before this moves forward.

Also, noticed some cli options enumerated in node.cc that are not in doc/api/cli.md. Specifically the following:

--napi-modules
--v8-pool-size=num
--experimental-modules
--experimental-vm-modules

Should I add those to the doc as part of this PR or create a different one?

vsemozhetbyt · 2018-04-08T22:48:10Z

I think as we are at this so deep and as we've already added some things from cli.md to node.cc, we can also do vice-versa. Just let's do it in a separate commit so that the first one remains reordering only, to not overcomplicate it.

willhayslett · 2018-04-08T23:03:28Z

Got it, @vsemozhetbyt!

vsemozhetbyt · 2018-04-08T23:38:03Z

So, I do not know C++, so the second commit needs to be reviewed by someone else. Maybe @Trott?
I just can find out this diffs resume with "sort-all-lines-and-compare" hack described above:

+         "  --                         indicate the end of node options\n"

+         "  -h, --help                 print node command line options\n"

+#if defined(NODE_HAVE_I18N_SUPPORT)
+         "NODE_PRESERVE_SYMLINKS       set to 1 to preserve symbolic links\n"
+         "                             when resolving and caching modules\n"
+#endif

(TBH, I do not know why NODE_HAVE_I18N_SUPPORT affects NODE_PRESERVE_SYMLINKS. Is this true?)

-         "  -                          script read from stdin (default; "
-         "interactive mode if a tty)\n"
+         "  -                          script read from stdin (default; \n"
+         "                             interactive mode if a tty)\n"

-    "--require", "-r",
+    "--require",
...
+    "-r",

And also these added and deleted lines in various places:

-#endif  /* NODE_FIPS_MODE */
-#endif /* HAVE_OPENSSL */

+#endif  // defined(NODE_HAVE_I18N_SUPPORT)
+#endif  // defined(NODE_HAVE_I18N_SUPPORT)
+#endif  // HAVE_INSPECTOR
+#endif  // HAVE_OPENSSL
+#endif  // HAVE_OPENSSL
+#endif  // HAVE_OPENSSL
+#endif  // HAVE_OPENSSL
+#endif  // NODE_FIPS_MODE
+#if defined(NODE_HAVE_I18N_SUPPORT)
+#if defined(NODE_HAVE_I18N_SUPPORT)
+#if HAVE_OPENSSL
+#if HAVE_OPENSSL
+#if HAVE_OPENSSL

vsemozhetbyt · 2018-04-08T23:45:34Z

Intermediate full CI to check the current stage: https://ci.nodejs.org/job/node-test-pull-request/14131/

vsemozhetbyt · 2018-04-09T00:49:29Z

One fail of parallel/test-http2-stream-client may be unrelated.

Trott · 2018-04-09T02:06:37Z

One fail of parallel/test-http2-stream-client may be unrelated.

Indeed, appears unrelated. Should re-run node-test-commit-linux just to set a good example if nothing else. (Well, that and I think our current governance rules require it.) But I can't restart or add any node-test-commit-linux jobs right now. They just disappear in CI. @nodejs/build

rvagg · 2018-04-09T02:38:28Z

I found a job (git-clean-rpi) that had a big queue cause old machines it was trying to run had been removed. I've fixed that up for now and new runs seem to be going through. I'm going to restart jenkins when I see it idle but for now it seems to be working OK.

willhayslett · 2018-04-09T03:20:29Z

(TBH, I do not know why NODE_HAVE_I18N_SUPPORT affects NODE_PRESERVE_SYMLINKS. Is this true?)

I'm curious about this as well. I added that check because the --preserve-symlinks option likewise appears to be wrapped in a preprocessor check for NODE_HAVE_I18N_SUPPORT:

-#if defined(NODE_HAVE_I18N_SUPPORT)
-         "  --icu-data-dir=dir         set ICU data load path to dir\n"
-         "                             (overrides NODE_ICU_DATA)\n"
-#if !defined(NODE_HAVE_SMALL_ICU)
-         "                             note: linked-in ICU data is present\n"
-#endif
-         "  --preserve-symlinks        preserve symbolic links when resolving\n"
-         "  --experimental-modules     experimental ES Module support\n"
-         "                             and caching modules\n"
-         "  --experimental-vm-modules  experimental ES Module support\n"
-         "                             in vm module\n"
-#endif

I may be reading the code wrong (which very well may be the case as I don't know c++ either), but if NODE_HAVE_I18N_SUPPORT doesn't impact --preserve-symlinks (or --experimental-modules and --experimental-vm-modules), there may be a pre-existing bug.

rvagg · 2018-04-09T03:50:21Z

node-test-pull-request @ https://ci.nodejs.org/job/node-test-pull-request/14133/

I think you need to also edit doc/node.1 for completeness here, it has the same list of arguments in the same order I think. So you'd better get your roff on and figure that one out too (man -l doc/node.1 to test it btw)

willhayslett · 2018-04-09T04:00:25Z

haha yep @rvagg! Was going to ask if that needed to be done as well. Started looking at mdoc and roff... should be fun! Thanks for the test command!

willhayslett · 2018-04-09T05:34:16Z

Do we have a way to perform tests against different versions of the compiled binary? I'd like to validate the latest commit against different build configurations. Specifically, the output of the PrintHelp() function changes based on the following build flags:

HAVE_OPENSSL: --without-ssl
NODE_FIPS_MODE: --openssl-fips
NODE_HAVE_I18N_SUPPORT: --with-icu-path
NODE_HAVE_SMALL_ICU: --with-intl; --without-intl
HAVE_INSPECTOR: --without-inspector
NODE_OPENSSL_CERT_STORE: --openssl-use-def-ca-store
NODE_WITHOUT_NODE_OPTIONS: --without-node-options

Is there any way to test against these with the current toolset?

vsemozhetbyt · 2018-04-13T03:17:03Z

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

richardlau · 2018-04-13T04:40:45Z

test/parallel/test-cli-node-print-help.js

+function testForSubstring(options) {
+  if (options.compileConstant) {
+    options.flags.forEach((flag) => {
+      assert.strictEqual(true, stdOut.indexOf(flag) !== -1);


Can you swap the arguments around? assert arguments are actual, expected.

richardlau · 2018-04-13T04:41:10Z

test/parallel/test-cli-node-print-help.js

+    });
+  } else {
+    options.flags.forEach((flag) => {
+      assert.strictEqual(-1, stdOut.indexOf(flag));


Can you swap the arguments around? assert arguments are actual, expected.

richardlau · 2018-04-13T04:43:13Z

test/parallel/test-cli-node-print-help.js

+function startPrintHelpTest() {
+  exec('./node --help', common.mustCall((err, stdout, stderr) => {
+    if (err) {
+      assert.ifError(err);


This doesn't need to be wrapped in if (err) {}.

richardlau · 2018-04-13T04:47:18Z

test/parallel/test-cli-node-print-help.js

+
+
+function startPrintHelpTest() {
+  exec('./node --help', common.mustCall((err, stdout, stderr) => {


Maybe use process.execPath?

richardlau · 2018-04-13T04:51:49Z

Test failing on Windows, e.g., https://ci.nodejs.org/job/node-test-binary-windows/16492/COMPILED_BY=vs2017,RUNNER=win10,RUN_SUBSET=1/console:

not ok 64 parallel/test-cli-node-print-help
  ---
  duration_ms: 0.140
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:589
        throw newErr;
        ^
    
    AssertionError [ERR_ASSERTION]: ifError got unwanted exception: Command failed: ./node --help
    '.' is not recognized as an internal or external command,
    operable program or batch file.
    
        at ChildProcess.exithandler (child_process.js:282:12)
        at ChildProcess.emit (events.js:182:13)
        at maybeClose (internal/child_process.js:944:16)
        at Process.ChildProcess._handle.onexit (internal/child_process.js:233:5)
  ...

Trott · 2018-04-13T05:52:25Z

Test failing on Windows

@willhayslett Using process.execPath (as @richardlau suggests in a different comment) would seem to fix that.

Trott

I'm putting the "Request Changes" red X here until the test is fixed. I don't want anyone to accidentally land this.

Alphabetizing the command line options and environment variables in doc/api/cli.md for consistency and readability. Refs: nodejs#19814

Updated the node.PrintHelp() function to return command line options and environment variables sorted in ascii order. Additionally, added missing options as sourced from doc/api/cli.md. Options added include "--", "--help" and the "NODE_PRESERVE_SYMLINKS" environment variable. Also updated the comments in the PrintHelp() method to C++ style. Fixes: nodejs#19814

Updated doc/api/cli.md to include command line options being printed in the node.PrintHelp() function in src/node.cc but weren't otherwise documented in the cli spec. Options added include:: --napi-modules --v8-pool-size=num --experimental-modules --experimental-vm-modules

Ascii sorted the node man page command line options. This change brings sort order consistency between the cli options displayed in doc/node.1 and the cli options enumerated in other areas of the project. Also rearranged the language for "--use-bundled-ca, --use-openssl-ca" to correspond with the order of the options as displayed. Fixes: nodejs#19814

Created tests to validate that the newly ascii sorted cli options in the node.PrintHelp() function are being returned according to build configurations as expected.

willhayslett · 2018-04-13T12:44:04Z

Thanks @richardlau and @Trott, changes committed.

vsemozhetbyt · 2018-04-13T12:59:36Z

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

vsemozhetbyt · 2018-04-13T14:32:40Z

Is one fail of parallel/test-http-server-keep-alive-timeout in one FreeBSD machine unrelated? Are we good to land?

Trott · 2018-04-13T18:28:20Z

@vsemozhetbyt Sure seems like it ought to be unrelated, but no landing without a green CI everywhere, so here's a FreeBSD re-run: https://ci.nodejs.org/job/node-test-commit-freebsd/16998/

test fixed

Trott · 2018-04-13T18:47:24Z

This time a different test failed on FreeBSD, also seemingly unrelated.

Trying again: https://ci.nodejs.org/job/node-test-commit-freebsd/16999/

vsemozhetbyt · 2018-04-13T19:48:35Z

Rerun CI is green. I will land soon if nobody objects.

Trott · 2018-04-13T20:09:50Z

Last one was green. 👍

* Alphabetize the command line options and environment variables in doc/api/cli.md for consistency and readability. * Update doc/api/cli.md to include command line options being printed in the `node.PrintHelp()` function in src/node.cc but weren't otherwise documented in the cli spec. Options added include: --napi-modules --v8-pool-size=num --experimental-modules --experimental-vm-modules * ASCII sort the node man page command line options. This change brings sort order consistency between the cli options displayed in doc/node.1 and the cli options enumerated in other areas of the project. Also rearrange the language for `--use-bundled-ca`, `--use-openssl-ca` to correspond with the order of the options as displayed. * Update `node.PrintHelp()` function to return command line options and environment variables sorted in ASCII order. Additionally, add missing options as sourced from doc/api/cli.md. Options added include `--`, `--help` and the `NODE_PRESERVE_SYMLINKS` environment variable. Also update the comments in the `node.PrintHelp()` method to C++ style. * Create tests to validate that the newly ASCII sorted cli options in the `node.PrintHelp()` function are being returned according to build configurations as expected. PR-URL: #19878 Refs: #19814 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: James M Snell <[email protected]>

vsemozhetbyt · 2018-04-13T20:35:41Z

Squashed and landed in a43e889

@willhayslett Thank you for all the heavy lifting!
Thanks to all reviewers for the time and toil.

willhayslett · 2018-04-13T20:55:31Z

Woo hoo! Thanks, everyone!

* Alphabetize the command line options and environment variables in doc/api/cli.md for consistency and readability. * Update doc/api/cli.md to include command line options being printed in the `node.PrintHelp()` function in src/node.cc but weren't otherwise documented in the cli spec. Options added include: --napi-modules --v8-pool-size=num --experimental-modules --experimental-vm-modules * ASCII sort the node man page command line options. This change brings sort order consistency between the cli options displayed in doc/node.1 and the cli options enumerated in other areas of the project. Also rearrange the language for `--use-bundled-ca`, `--use-openssl-ca` to correspond with the order of the options as displayed. * Update `node.PrintHelp()` function to return command line options and environment variables sorted in ASCII order. Additionally, add missing options as sourced from doc/api/cli.md. Options added include `--`, `--help` and the `NODE_PRESERVE_SYMLINKS` environment variable. Also update the comments in the `node.PrintHelp()` method to C++ style. * Create tests to validate that the newly ASCII sorted cli options in the `node.PrintHelp()` function are being returned according to build configurations as expected. PR-URL: #19878 Refs: #19814 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: James M Snell <[email protected]>

* Alphabetize the command line options and environment variables in doc/api/cli.md for consistency and readability. * Update doc/api/cli.md to include command line options being printed in the `node.PrintHelp()` function in src/node.cc but weren't otherwise documented in the cli spec. Options added include: --napi-modules --v8-pool-size=num --experimental-modules --experimental-vm-modules * ASCII sort the node man page command line options. This change brings sort order consistency between the cli options displayed in doc/node.1 and the cli options enumerated in other areas of the project. Also rearrange the language for `--use-bundled-ca`, `--use-openssl-ca` to correspond with the order of the options as displayed. * Update `node.PrintHelp()` function to return command line options and environment variables sorted in ASCII order. Additionally, add missing options as sourced from doc/api/cli.md. Options added include `--`, `--help` and the `NODE_PRESERVE_SYMLINKS` environment variable. Also update the comments in the `node.PrintHelp()` method to C++ style. * Create tests to validate that the newly ASCII sorted cli options in the `node.PrintHelp()` function are being returned according to build configurations as expected. PR-URL: nodejs#19878 Refs: nodejs#19814 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added cli Issues and PRs related to the Node.js command line interface. doc Issues and PRs related to the documentations. labels Apr 8, 2018

willhayslett force-pushed the node-sort-doc-api-cli branch from 0936cf3 to 27eb437 Compare April 8, 2018 17:54

vsemozhetbyt approved these changes Apr 8, 2018

View reviewed changes

willhayslett force-pushed the node-sort-doc-api-cli branch from 27eb437 to d5a5eb3 Compare April 8, 2018 22:17

willhayslett force-pushed the node-sort-doc-api-cli branch from d5a5eb3 to ca84066 Compare April 9, 2018 05:18

richardlau reviewed Apr 13, 2018

View reviewed changes

Trott previously requested changes Apr 13, 2018

View reviewed changes

willhayslett added 5 commits April 13, 2018 07:42

doc: sort cli cmd options and env vars

83c96b9

Alphabetizing the command line options and environment variables in doc/api/cli.md for consistency and readability. Refs: nodejs#19814

test: create tests for newly sorted cli options

dd33653

Created tests to validate that the newly ascii sorted cli options in the node.PrintHelp() function are being returned according to build configurations as expected.

willhayslett force-pushed the node-sort-doc-api-cli branch from e7e685a to dd33653 Compare April 13, 2018 12:42

refack added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 13, 2018

vsemozhetbyt closed this Apr 13, 2018

richardlau mentioned this pull request Apr 16, 2018

documentation of --napi-modules #20082

Closed

richardlau mentioned this pull request Apr 25, 2018

n-api: update cli documentation #20301

Closed

3 tasks

vsemozhetbyt mentioned this pull request Aug 11, 2018

cli: fix flags on help output #22271

Merged

4 tasks



		function startPrintHelpTest() {
		exec('./node --help', common.mustCall((err, stdout, stderr) => {

doc, src: sort cli cmd options and env vars #19878

doc, src: sort cli cmd options and env vars #19878

Conversation

willhayslett commented Apr 8, 2018

Checklist

vsemozhetbyt commented Apr 8, 2018

vsemozhetbyt commented Apr 8, 2018 • edited Loading

vsemozhetbyt commented Apr 8, 2018 • edited Loading

willhayslett commented Apr 8, 2018 • edited Loading

vsemozhetbyt commented Apr 8, 2018 • edited Loading

willhayslett commented Apr 8, 2018

vsemozhetbyt commented Apr 8, 2018 • edited Loading

willhayslett commented Apr 8, 2018

vsemozhetbyt commented Apr 8, 2018 • edited Loading

vsemozhetbyt commented Apr 8, 2018

vsemozhetbyt left a comment

Choose a reason for hiding this comment

vsemozhetbyt commented Apr 8, 2018

vsemozhetbyt commented Apr 8, 2018

willhayslett commented Apr 8, 2018

vsemozhetbyt commented Apr 8, 2018

willhayslett commented Apr 8, 2018

vsemozhetbyt commented Apr 8, 2018 • edited Loading

vsemozhetbyt commented Apr 8, 2018

vsemozhetbyt commented Apr 9, 2018

Trott commented Apr 9, 2018

rvagg commented Apr 9, 2018

willhayslett commented Apr 9, 2018

rvagg commented Apr 9, 2018

willhayslett commented Apr 9, 2018

willhayslett commented Apr 9, 2018

vsemozhetbyt commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardlau commented Apr 13, 2018

Trott commented Apr 13, 2018

Trott left a comment

Choose a reason for hiding this comment

willhayslett commented Apr 13, 2018

vsemozhetbyt commented Apr 13, 2018

vsemozhetbyt commented Apr 13, 2018

Trott commented Apr 13, 2018

Trott commented Apr 13, 2018

vsemozhetbyt commented Apr 13, 2018

Trott commented Apr 13, 2018

vsemozhetbyt commented Apr 13, 2018

willhayslett commented Apr 13, 2018

vsemozhetbyt commented Apr 8, 2018 •

edited

Loading

vsemozhetbyt commented Apr 8, 2018 •

edited

Loading

willhayslett commented Apr 8, 2018 •

edited

Loading

vsemozhetbyt commented Apr 8, 2018 •

edited

Loading

vsemozhetbyt commented Apr 8, 2018 •

edited

Loading

vsemozhetbyt commented Apr 8, 2018 •

edited

Loading

vsemozhetbyt commented Apr 8, 2018 •

edited

Loading