src: port --bash-completion to C++ by joyeecheung · Pull Request #25901 · nodejs/node · GitHub

So that it gets handle earlier and faster during the bootstrap
process.

Drive-by fixes:

Remove [has_eval_string] and [ssl_openssl_cert_store] from
the completion output

Before:

_node_complete() {
  local cur_word options
  cur_word="${COMP_WORDS[COMP_CWORD]}"
  if [[ "${cur_word}" == -* ]] ; then
    COMPREPLY=( $(compgen -W '--abort-on-uncaught-exception --experimental-report --perf-basic-prof --max-old-space-size --diagnostic-report-verbose --inspect-brk-node --inspect-port --diagnostic-report-filename --diagnostic-report-uncaught-exception --track-heap-objects --diagnostic-report-signal --tls-v1.1 --napi-modules --inspect-brk --tls-v1.0 --redirect-warnings --print --trace-deprecation --trace-event-file-pattern --check --perf-prof --preserve-symlinks --no-warnings --debug --no-deprecation --trace-warnings --expose-internals --diagnostic-report-on-signal --diagnostic-report-directory --pending-deprecation --experimental-worker --trace-sync-io --diagnostic-report-on-fatalerror --tls-cipher-list --no-force-async-hooks-checks --inspect --eval --loader --use-openssl-ca --preserve-symlinks-main --interactive --icu-data-dir --v8-options --require --use-bundled-ca --experimental-policy --version --experimental-vm-modules --prof-process --max-http-header-size [has_eval_string] --throw-deprecation --completion-bash --help --zero-fill-buffers --v8-pool-size [ssl_openssl_cert_store] --experimental-modules --http-parser --openssl-config --trace-event-categories --security-reverts --experimental-repl-await --stack-trace-limit --debug-brk --title --debug-port --prof-process --debug= -p -pe -v --inspect-brk= -i --print <arg> --inspect= --debug-brk= -e --inspect-brk-node= -c -h -r --trace-events-enabled' -- "${cur_word}") )
    return 0
  else
    COMPREPLY=( $(compgen -f "${cur_word}") )
    return 0
  fi
}
complete -F _node_complete node node_g

After:

_node_complete() {
  local cur_word options
  cur_word="${COMP_WORDS[COMP_CWORD]}"
  if [[ "${cur_word}" == -* ]] ; then
    COMPREPLY=( $(compgen -W '--abort-on-uncaught-exception --experimental-report --perf-basic-prof --max-old-space-size --diagnostic-report-verbose --inspect-brk-node --inspect-port --diagnostic-report-filename --diagnostic-report-uncaught-exception --track-heap-objects --diagnostic-report-signal --tls-v1.1 --napi-modules --inspect-brk --tls-v1.0 --redirect-warnings --print --trace-deprecation --trace-event-file-pattern --check --perf-prof --preserve-symlinks --no-warnings --debug --no-deprecation --trace-warnings --expose-internals --diagnostic-report-on-signal --diagnostic-report-directory --pending-deprecation --experimental-worker --trace-sync-io --diagnostic-report-on-fatalerror --tls-cipher-list --no-force-async-hooks-checks --inspect --eval --loader --use-openssl-ca --preserve-symlinks-main --interactive --icu-data-dir --v8-options --require --use-bundled-ca --experimental-policy --version --experimental-vm-modules --prof-process --max-http-header-size --throw-deprecation --completion-bash --help --zero-fill-buffers --v8-pool-size --experimental-modules --http-parser --openssl-config --trace-event-categories --security-reverts --experimental-repl-await --stack-trace-limit --debug-brk --title --debug-port --prof-process --debug= -p -pe -v --inspect-brk= -i --print <arg> --inspect= --debug-brk= -e --inspect-brk-node= -c -h -r --trace-events-enabled' -- "${cur_word}") )
    return 0
  else
    COMPREPLY=( $(compgen -f "${cur_word}") )
    return 0
  fi
}
complete -F _node_complete node node_g

Checklist

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

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2496/pipeline

I'm wondering if keeping this in JS is worth it, because that makes it more accessible to people? I realize it's slower, but we encourage people to write the output to a file anyway, so I wouldn't do this for performance reasons

This could also be a member of OptionsParser, right?

It can, but so is GetOptions() so I figured it's better to keep them consistent

@addaleax IMO it is an overkill to implement --bash-completion in JS, and being in JS does not necessarily mean it's more accessible. This flag essentially just formats structured data from the binary into a string and prints it to stdout, there is no user input or error handling necessary. Implementing it in JS means it has to depend on a working v8 isolate and an Environment, and rely on proper serialization of a complicated v8::Object as well as a working stdout stream, those are all too complicated for a simple feature like --bash-completion. (In fact to remove [has_eval_string] and [ssl_openssl_cert_store] my first action was to debug GetOptions() since that's where all the complexities were, it was much easier to debug the loop in GetBashCompletion() to be honest, so moving the formatting in JS did not really make the implementation any more accessible if the source of data is in C++)

Being handled later in time (after bootstrap/node.js instead of directly in node::Init) also means that there is a mental burden when we add branches in the bootstrap (e.g. node-report) even though they do not really make sense for --bash-completion so we might as well just deal with it earlier and focus on cases that are more common in the bootstrap process - this is also the case for things like node --version.

@addaleax: is your comment in #25901 (review) blocking?

@joyeecheung No, it’s not :)

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

@joyeecheung this needs a rebase

Ping @joyeecheung

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

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

So that it gets handle earlier and faster during the bootstrap process. Drive-by fixes: - Remove `[has_eval_string]` and `[ssl_openssl_cert_store]` from the completion output - Set `kProfProcess` execution mode for `--prof-process` instead of `kPrintBashProcess` which is removed in this patch. - Append new line to the end of the output of --bash-completion

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

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

Fixed the Windows failure. @devsnek @jasnell @addaleax can you take a look again? Thanks!

Ping @devsnek @jasnell @addaleax

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

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

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

@joyeecheung Since you pinged me – the code changes look good to me, but I’d still have a mild preference to keep this in JS because that does seem to be less complex to me. But as far as I’m concerned, this PR is ready to land 👍 (I can’t make out what the Windows failure is, so I’ll just start a resume CI…)

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

ping @joyeecheung

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

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

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

So that it gets handle earlier and faster during the bootstrap process. Drive-by fixes: - Remove `[has_eval_string]` and `[ssl_openssl_cert_store]` from the completion output - Set `kProfProcess` execution mode for `--prof-process` instead of `kPrintBashProcess` which is removed in this patch. - Append new line to the end of the output of --bash-completion PR-URL: #25901 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in 403c84a 🎉

So that it gets handle earlier and faster during the bootstrap process. Drive-by fixes: - Remove `[has_eval_string]` and `[ssl_openssl_cert_store]` from the completion output - Set `kProfProcess` execution mode for `--prof-process` instead of `kPrintBashProcess` which is removed in this patch. - Append new line to the end of the output of --bash-completion PR-URL: #25901 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 3, 2019

joyeecheung added the cli Issues and PRs related to the Node.js command line interface. label Feb 3, 2019

addaleax reviewed Feb 3, 2019

View reviewed changes

devsnek approved these changes Feb 6, 2019

View reviewed changes

jasnell approved these changes Feb 18, 2019

View reviewed changes

joyeecheung force-pushed the cpp-bash branch from b23997e to 6635c6e Compare April 2, 2019 06:27

joyeecheung mentioned this pull request Apr 2, 2019

CI failures: 20190330-20190402 nodejs/reliability#22

Closed

refack mentioned this pull request Apr 2, 2019

flaky: parallel/test-http2-* on Windows #20750

Closed

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 3, 2019

joyeecheung removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 3, 2019

joyeecheung force-pushed the cpp-bash branch from 6635c6e to 2911595 Compare May 11, 2019 20:35

joyeecheung force-pushed the cpp-bash branch from 2911595 to 3190ae1 Compare May 11, 2019 20:43

fixup! src: port --bash-completion to C++

bdfabf4

fixup! fixup! src: port --bash-completion to C++

e8986f0

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:51

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 20, 2019

BridgeAR closed this Dec 25, 2019

BridgeAR mentioned this pull request Jan 7, 2020

v13.6.0 proposal #31238

Merged

targos mentioned this pull request Jan 15, 2020

v12.15.0 release proposal #31368

Closed

MylesBorins mentioned this pull request Feb 8, 2020

v12.16.0 proposal #31691

Merged

src: port --bash-completion to C++ by joyeecheung · Pull Request #25901 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: port --bash-completion to C++ - #25901

src: port --bash-completion to C++#25901joyeecheung wants to merge 3 commits intonodejs:masternodejs/node:masterfrom joyeecheung:cpp-bashjoyeecheung/node:cpp-bashCopy head branch name to clipboard

Conversation

joyeecheung commented Feb 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Feb 3, 2019

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax Feb 3, 2019

Choose a reason for hiding this comment

Uh oh!

joyeecheung Feb 17, 2019

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Feb 17, 2019

Uh oh!

addaleax commented Feb 17, 2019

Uh oh!

BridgeAR commented Mar 5, 2019

Uh oh!

BridgeAR commented Mar 27, 2019

Uh oh!

nodejs-github-bot commented Apr 2, 2019

Uh oh!

nodejs-github-bot commented Apr 3, 2019

Uh oh!

nodejs-github-bot commented May 11, 2019

Uh oh!

nodejs-github-bot commented May 12, 2019

Uh oh!

joyeecheung commented May 12, 2019

Uh oh!

joyeecheung commented May 20, 2019

Uh oh!

nodejs-github-bot commented May 20, 2019

Uh oh!

nodejs-github-bot commented May 20, 2019

Uh oh!

nodejs-github-bot commented May 22, 2019

Uh oh!

addaleax commented May 22, 2019

Uh oh!

nodejs-github-bot commented May 22, 2019

Uh oh!

fhinkel commented Oct 28, 2019

Uh oh!

nodejs-github-bot commented Dec 20, 2019

Uh oh!

nodejs-github-bot commented Dec 24, 2019

Uh oh!

nodejs-github-bot commented Dec 25, 2019

Uh oh!

BridgeAR commented Dec 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

src: port --bash-completion to C++#25901
joyeecheung wants to merge 3 commits into
nodejs:masterfrom
joyeecheung:cpp-bash

joyeecheung commented Feb 3, 2019 •
edited
Loading

joyeecheung commented Feb 6, 2019 •
edited
Loading