oh-my-bash
oh-my-bash copied to clipboard
Replace `which` with `command -v`
Debian has deprecated usage of which
, as per:
https://salsa.debian.org/debian/debianutils/-/commit/3a8dd10b4502f7bae8fc6973c13ce23fc9da7efb
The recommendation is now to use command -v
.
@nntoan can you take a look?
I got the same problem in Debian sid. https://linux.debian.bugs.dist.narkive.com/eCDBKTyB/bug-992401-debianutils-warning-message-usr-bin-which-this-version-of-which-is-deprecated-and-should- I suggest there are more 'which' (around 10 more) to replace in the code.
Thank you for the additional information.
@levilovearch I have fixed the remaining instances of which
in the repo.
@nntoan can you please take a look?
I also found the uses of `type CommandName &>/dev/null':
./completions/docker-compose.completion.sh:42: type compopt &>/dev/null && compopt -o nospace
./completions/docker.completion.sh:409: type compopt &>/dev/null && compopt -o nospace
./completions/git.completion.sh:158:if ! type _get_comp_words_by_ref >/dev/null 2>&1; then
./lib/base.sh:117: if type "mkisofs" > /dev/null; then
./lib/base.sh:268: type "$1" &> /dev/null ;
./plugins/nvm/nvm.plugin.sh:8:if ! type "nvm" &> /dev/null; then
./plugins/sdkman/sdkman.plugin.sh:7:if ! type "sdk" &> /dev/null; then
I think these are generally good fixes, but have you noticed the existing shell function
type_exists
for this purpose? I feel we should update the definition oftype_exists
to something more efficient liketype -P "$@" &>/dev/null
orcommand -v "$@" &>/dev/null
and then usetype_exists
everywhere, which is more consistent and idiomatic. What do you think?
I think this is a good idea. Do you mean that we should update the builtin bash function somehow? Or, is type_exists
something provided by ohmybash?
Do you mean that we should update the builtin bash function somehow? Or, is type_exists something provided by ohmybash?
Thank you for your reply! I meant the latter. Currently, the definition of type_exists
is given here. We can replace the function body with a more efficient implementation.
I found a related PR #171. I think we need to care about the differences between the builtin commands that can be used to detect the commands. I summarized in the following table which command can detect each type of the commands.
Command | file |
builtin |
function |
keyword |
alias |
---|---|---|---|---|---|
command -v -- CMD &>/dev/null |
v | v | v | v | v |
type -- CMD &>/dev/null |
v | v | v | v | v |
hash -- CMD &>/dev/null |
v | v | v | ||
type -P -- CMD &>/dev/null |
v | ||||
declare -F -- CMD &>/dev/null |
v | ||||
compgen -X '!CMD' -k &>/dev/null |
v | ||||
alias CMD &>/dev/null |
v | ||||
which -- CMD &>/dev/null |
v | ||||
(alias;declare -f) | which --i --read-functions -- CMD &>/dev/null (GNU) |
v | v | v |
Those different variations can be found here and there in the codebase, which I think should be made consistent. #171 implies that we might need to prepare several functions like type_exists
for different purposes.
Edit: Bash-it seems to prepare two shell functions, _command_exists
and _binary_exists
, for this purpose.
I have adopted the change made in #171 and then renamed type_exists
to _omb_util_command_exists
. I have also prepared _omb_util_binary_exists
for the older behavior using type -P "$1"
. @mchaker Can you update the PR by using the newly introduced functions (_omb_util_command_exists
or _omb_util_binary_exists
)?
It'd be great if you could also update other uses of command -v &>/dev/null
, type ... &>/dev/null
, hash ... &>/dev/null
, etc. but that's not mandatory.
Thank you so much @akinomyoga !
I will reply in detail once I return from holiday travel.
I appreciate your collaboration and work very much.
Thank you for your reply! OK! Have a good year and holidays!
I have adopted the change made in #171 and then renamed
type_exists
to_omb_util_command_exists
. I have also prepared_omb_util_binary_exists
for the older behavior usingtype -P "$1"
. @mchaker Can you update the PR by using the newly introduced functions (_omb_util_command_exists
or_omb_util_binary_exists
)?It'd be great if you could also update other uses of
command -v &>/dev/null
,type ... &>/dev/null
,hash ... &>/dev/null
, etc. but that's not mandatory.
Ok, so looking back on your comments, I am very grateful for your collaboration.
Now for the next step: You have made _omb_util_command_exists
and _omb_util_binary_exists
. I think that is a good idea.
Let me see if I am understanding what I should do next:
- Use
_omb_util_binary_exists
for all instances oftype -P
. - Use
_omb_util_command_exists
for all other instances of commands in the table from your earlier comment.
Is that correct?
Thank you!
Let me see if I am understanding what I should do next:
- Use
_omb_util_binary_exists
for all instances oftype -P
.- Use
_omb_util_command_exists
for all other instances of commands in the table from your earlier comment.Is that correct?
Basically, yes. Additionally, the tests declare -f
/ declare -F
are supposed to be replaced by _omb_util_function_exists
. Currently there are no replacements for alias CMD &>/dev/null
and compgen -k -X \!CMD
, so they should be kept as is for now (although I'm not sure if there are any instances of them).
Also, the rewriting is not strict as I don't think the existing instances of these commands are so carefully chosen. E.g., if you think _omb_util_binary_exists
is more appropriate for a specific instance of command -v
, etc., you can change it as you like.
Also, be careful that some of the codes are supposed to be executed before loading lib/utils.sh
or without loading lib/util.sh
(such as tools/{install,uninstall,upgrade}.sh
). In such cases, we cannot use these utilities defined in lib/utils.sh
but can directly write type -P
or command -v
. I have introduced a new function _omb_module_require
in #274 to control the loading order of lib/*
, so we can put _omb_module_require lib:utils
at the beginning of lib/*.sh
that rely on these utilities so that lib/utils.sh
is ensured to be loaded first, but I can take care of it after you made the changes if it is not so obvious for you.
I noticed a function command_exists
is also defined in lib/base.sh
and used in plugins/battery/battery.plugin.sh
. I think we can remove this function and replace it with _omb_util_command_exists
.
@akinomyoga Thank you for your patience -- I have attempted to make the changes requested. What do you think?
@mchaker Hi! If you are busy, I think I can take over the remaining changes in this PR. What do you think? Thank you!
@akinomyoga Hi Koichi! I apologize for the radio silence. I am still willing to finish the changes -- will go through the requested changes now.
@mchaker The changes are still incomplete for a long time. Or are you still working on it? Can I take over this PR?
@akinomyoga Yes, you can take over this PR. Thank you for your patience, I wish I could have completed it sooner.
Thanks!
I've squashed related commits and fixed the present problems.
There are many conflicts. I'm going to rebase it to the latest master
branch.
rebase done
@mchaker I think I have finished the changes that I mentioned before. Also, I have added some other changes and adjustments. I'll keep this PR open for a while. If you have any comments on the updated version of the PR, please feel free to leave them here.
@nntoan Could you review the PR? The repository setting seems to have been changed recently so that any approval is needed to merge the PR. (I can actually self-approve my changes and forcibly merge the PR, but that doesn't seem like the neat way.) We have unified all the existence tests of commands (which varied to use which
, type
, hash
, declare -f
, type -P
, command -v
, and alias
) in the utility functions _omb_util_{binary,command,function}_exists
. The exceptions are the scripts in the subdirectory tools
; They can be called as independent scripts, so the utility functions cannot be used there.
@nntoan Can I merge the PR if there are no comments or concerns?
@nntoan Can I merge the PR if there are no comments or concerns?
@akinomyoga Hey, yap seems all good to me. This can be merged, forgot to reply the thread earlier.
@nntoan Thank you very much for the quick reply!
Edit: I have merged it!
🙌🙏🥳 thank you so much!