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.

Sign up for GitHub

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

fix: remove empty arguments from launcher #1650

Merged
merged 4 commits into from
Feb 18, 2020
Merged

fix: remove empty arguments from launcher #1650

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1ac86fb
fix: remove empty arguments from launcher
purkhusid Feb 17, 2020
5597ee8
fix: remove duplication and add tests
purkhusid Feb 18, 2020
a22947a
fix: remove accidental commit of vscode settings
purkhusid Feb 18, 2020
22ffb6e
fix: remove unecessary change
purkhusid Feb 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions internal/node/launcher.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,13 +224,28 @@ if [ "${EXPECTED_EXIT_CODE}" -eq "0" ]; then
# handled by the node process.
# If we had merely forked a child process here, we'd be responsible
# for forwarding those OS interactions.
exec "${node}" "${LAUNCHER_NODE_OPTIONS[@]:-}" "${USER_NODE_OPTIONS[@]:-}" "${MAIN}" "${ARGS[@]:-}"
if (( ${#ARGS[@]} )); then
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe if (( ${#ARGS} )); then is equivalent and sufficient here.

The duplication is not great. How about something like:

command="${node}" "${LAUNCHER_NODE_OPTIONS[@]:-}" "${USER_NODE_OPTIONS[@]:-}" "${MAIN}"
# If there are no arguments we skip the arguments because some programs
# such as grpc_tools_node_protoc_plugin in the grpc-tools do not handle empty arguments well
if (( ${#ARGS} )); then
  command="${command}" "${ARGS[@]:-}"
fi
exec "${command}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a issue with using set -u and empty arrays with older versions of bash.
See more information here: https://stackoverflow.com/questions/7577052/bash-empty-array-expansion-with-set-u

# If the there are any arguments we execute the program with arguments
exec "${node}" "${LAUNCHER_NODE_OPTIONS[@]:-}" "${USER_NODE_OPTIONS[@]:-}" "${MAIN}" "${ARGS[@]:-}"
else
# If there are no arguments we skip the arguments because
# some programs do not handle empty arguments well
exec "${node}" "${LAUNCHER_NODE_OPTIONS[@]:-}" "${USER_NODE_OPTIONS[@]:-}" "${MAIN}"
fi
# exec terminates execution of this shell script, nothing later will run.
fi

set +e
"${node}" "${LAUNCHER_NODE_OPTIONS[@]:-}" "${USER_NODE_OPTIONS[@]:-}" "${MAIN}" "${ARGS[@]:-}"
RESULT="$?"
if (( ${#ARGS[@]} )); then
# If the there are any arguments we execute the program with arguments
"${node}" "${LAUNCHER_NODE_OPTIONS[@]:-}" "${USER_NODE_OPTIONS[@]:-}" "${MAIN}" "${ARGS[@]:-}"
RESULT="$?"
else
# If there are no arguments we skip the arguments because
# some programs do not handle empty arguments well
"${node}" "${LAUNCHER_NODE_OPTIONS[@]:-}" "${USER_NODE_OPTIONS[@]:-}" "${MAIN}"
RESULT="$?"
fi
set -e

if [ ${RESULT} != ${EXPECTED_EXIT_CODE} ]; then
Expand Down