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: port --bash-completion to C++ #25901

Closed
wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Feb 3, 2019

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

@nodejs-github-bot 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 joyeecheung added the cli Issues and PRs related to the Node.js command line interface. label Feb 3, 2019
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

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

@@ -400,6 +401,7 @@ class OptionsParser {
friend class OptionsParser;

friend void GetOptions(const v8::FunctionCallbackInfo<v8::Value>& args);
friend std::string GetBashCompletion();
Copy link
Member

Choose a reason for hiding this comment

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

This could also be a member of OptionsParser, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@joyeecheung
Copy link
Member Author

joyeecheung commented Feb 6, 2019

@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.

@joyeecheung
Copy link
Member Author

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

@addaleax
Copy link
Member

@BridgeAR
Copy link
Member

BridgeAR commented Mar 5, 2019

@joyeecheung this needs a rebase

@BridgeAR
Copy link
Member

Ping @joyeecheung

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 3, 2019
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 3, 2019
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
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

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

@joyeecheung
Copy link
Member Author

Ping @devsnek @jasnell @addaleax

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member

@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…)

@nodejs-github-bot
Copy link
Collaborator

@fhinkel
Copy link
Member

fhinkel commented Oct 28, 2019

ping @joyeecheung

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 20, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

BridgeAR pushed a commit that referenced this pull request Dec 25, 2019
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR
Copy link
Member

Landed in 403c84a 🎉

@BridgeAR BridgeAR closed this Dec 25, 2019
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
targos pushed a commit that referenced this pull request Jan 14, 2020
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants