Support zsh in call_host.sh
#42
Conversation
…NCNAME[1]) instead of the helper's own name.
|
@clelange thanks for this contribution! tagging @NJManganelli here because he had also expressed interest in zsh support. I won't have time to review this in depth until next week, but I wanted to acknowledge receipt. |
|
In the meantime, you can try to placate shellcheck... |
|
I had hacked together a minimal translation and had no time to clean it up, but even at a glance this is better and more functional. I can hopefully test it as a replacement for my hacked version later |
This should be OK now. |
| @@ -1,4 +1,5 @@ | |||
| #!/bin/bash | |||
| # Print each command before executing it (for debugging) | |||
There was a problem hiding this comment.
is this comment accurate / needed?
There was a problem hiding this comment.
Sorry, a leftover from debugging, will remove
| # get last path component if ps returns full path | ||
| p="$(ps -p $$ -o comm= 2>/dev/null | awk -F/ '{print $NF}')" | ||
| case "$p" in | ||
| zsh) return 0 ;; |
There was a problem hiding this comment.
because there is only a zsh case here, in the bash case, this will proceed through the fallback below before returning non-zero. I would prefer stopping here if bash is directly detected.
| # bash | ||
| export_func(){ | ||
| [ -n "$1" ] || return | ||
| eval "export -f $1" 2>/dev/null || true |
There was a problem hiding this comment.
eval is not necessary here. The shellcheck warning should be handled in either of the following ways:
- put
# shellcheck disable=SC2163above this line - use
export -f ${1?}as noted at https://www.shellcheck.net/wiki/SC2163
There was a problem hiding this comment.
Since there's already a check for the existence of ${1}$, I'll go with the former
| } | ||
| declare_assoc(){ | ||
| # create named associative array in bash | ||
| eval "declare -A $1" |
There was a problem hiding this comment.
doesn't seem like eval should be necessary here either
There was a problem hiding this comment.
I think it's needed because declare expects a literal variable name and not a variable containing the name. Are you able to test this quickly? Otherwise, I can check next week.
There was a problem hiding this comment.
the following works fine for me:
TEST1=TEST2
declare -A $TEST1
TEST2[foo]=bar
echo "${TEST2[@]}"output:
barThere was a problem hiding this comment.
OK, thanks, I'll remove the eval. Just wanted to make sure I'm not breaking the bash part.
| } | ||
| # declare an associative array (zsh) - create a named array using eval so dynamic name works | ||
| declare_assoc(){ | ||
| eval "typeset -A $1" |
There was a problem hiding this comment.
check if eval necessary here
| echo "Warning: unsupported value ${!VAR_TO_VALIDATE} for $VAR_TO_VALIDATE; disabling" | ||
| # retrieve the value of the named variable in a way that works in bash and zsh | ||
| VARVAL="$(eval "printf '%s' \"\${$VAR_TO_VALIDATE}\"")" | ||
| # check allowed values in a POSIX-compatible way |
There was a problem hiding this comment.
zsh really doesn't support =~?
There was a problem hiding this comment.
(the benefit of this construct is scaling: more allowed values can be added without repeating "$VARVAL" !=)
There was a problem hiding this comment.
It does support it, but it's a bit tricky because of differences in quoting and word-splitting, so this is a more robust implementation. One could switch to case or loop/grep to avoid repeating "$VARVAL" !=. What's your take?
There was a problem hiding this comment.
Case would be okay with me.
| # args: varname value [sep] | ||
| varname="$1"; val="$2"; sep="${3:-:}" | ||
| # retrieve current value portably | ||
| cur="$(eval "printf '%s' \"\${${varname}:-}\"")" |
There was a problem hiding this comment.
maybe should be a function, since it's used repeatedly (also for $VAR_TO_VALIDATE above)
| APPTAINERENV_HOSTFNS="$( ( IFS=: | ||
| for d in $PATH; do | ||
| [ -d "$d" ] || continue | ||
| for f in "$d"/condor_* "$d"/eos*; do |
There was a problem hiding this comment.
would prefer to avoid repetition here (specifying condor and eos twice, since the list could grow in the future). put these in an array, then either create the grep expression (first case) or use a nested loop (second case)?
| # portable retrieval of function source and re-definition under a new name | ||
| if is_zsh; then | ||
| # zsh: use `functions` to get the definition | ||
| if functions "$1" >/dev/null 2>&1; then | ||
| fnsrc="$(functions "$1" 2>/dev/null)" | ||
| else | ||
| return | ||
| fi | ||
| else | ||
| # bash: use declare -f | ||
| if declare -f "$1" >/dev/null 2>&1; then | ||
| fnsrc="$(declare -f "$1")" | ||
| else | ||
| return | ||
| fi | ||
| fi |
There was a problem hiding this comment.
this is fairly repetitive. suggestion:
- make
get_function()functions for both bash and zsh (usingdeclare -fandfunctions, resp.) in the "compatibility helpers" section at the top - here, just call:
then the remaining lines below
fnsrc="$(get_function "$1")" if [ -z "$fnsrc" ]; then return; fi
|
Thanks for the careful review. I'm not sure I'll be able to address everything this week, but if not, I'll pick this up in the second half of next week. |
3 participants
I'm using zsh for most of my work, which is currently not supported by the
call_host.shscript. I've made it work by identifying the bash-specific parts and changing those to functions. I've also introduced a function to identify if zsh is used and another one to prevent duplicating the bind paths when accidentally sourcing thecall_host.shscript more than once.