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

SC2230 - command -v is not a direct replacement for which #1162

Closed
1 task done
arth1 opened this issue Mar 28, 2018 · 28 comments
Closed
1 task done

SC2230 - command -v is not a direct replacement for which #1162

arth1 opened this issue Mar 28, 2018 · 28 comments

Comments

@arth1
Copy link

arth1 commented Mar 28, 2018

For bugs

  • Rule Id (if any, e.g. SC1000): SC2230
  • My shellcheck version (shellcheck --version or "online"): 4.4.7 (latest from git)
  • I tried on shellcheck.net and verified that this is still a problem on the latest commit

Here's a snippet or screenshot that shows the problem:

#!/bin/sh

which "$0"
/usr/bin/which "$0"
command -v "$0"

Here's what shellcheck currently says:

In test.sh line 3:
which "$0"
^-- SC2230: which is non-standard. Use builtin 'command -v' instead.

In test.sh line 4:
/usr/bin/which "$0"
^-- SC2230: which is non-standard. Use builtin 'command -v' instead.

Here's what I wanted or expected to see:

Because "command -v" does not do the same as which, I would have liked to see nothing.
When running the above (from /tmp), I get:
/tmp/test.sh
/tmp/test.sh
./test.sh

While "which" may not be standard, it is commonly used to get the full path.
At the very least I would like to not get warned when I specifically use the path as in the third command - then it's fairly certain what the user wants, which won't be the common bash alias.

@Dmole
Copy link

Dmole commented May 14, 2018

You could use 'pwd'.

'which' is not posix
http://pubs.opengroup.org/onlinepubs/9699919799/idx/utilities.html
and not gnucore
https://en.m.wikipedia.org/wiki/List_of_GNU_Core_Utilities_commands

but it sure seems common to me.. where is it not?
also shellcheck is not warning about far less common things like Java... seems like a false positive to me.

@brodygov
Copy link

command -v also has slightly different behavior since it will return success for shell built-ins and functions that do not exist as standalone commands, whereas which only searches the PATH.

For example:

$ which if
# exits nonzero

$ command -v if
if
# exits zero

$ foo() { true ; }
$ which foo
# exits nonzero
$ command -v foo
foo
# exits zero

xwmx added a commit to xwmx/airport that referenced this issue Aug 26, 2018
https://github.com/koalaman/shellcheck/wiki/SC2230

See: "SC2230 - command -v is not a direct replacement for which #1162"
  koalaman/shellcheck#1162
@pedroarthur
Copy link

Besides that, which a b c exits zero if all of a, b, and c exist. Meanwhile, command -v a b c exits zero if any of a, b, or c exists.

@pdietl
Copy link

pdietl commented Jan 29, 2019

So do we think that this check should be removed?

@brodygov
Copy link

brodygov commented Feb 1, 2019

👍 I would favor removal.

@Light-Wizzard
Copy link

which can be replaced with type -p,
but it still needs the -x, I do not get any warnings using this:
[[ -x "$(type -p dir 2>/dev/null)" ]]

@leagris
Copy link

leagris commented Feb 18, 2019

An alternative to which command, when you need to know the path of a non built-in command:

type -P ls
/bin/ls

@oliv3r
Copy link

oliv3r commented Feb 23, 2019

While which can certainly be used if a user decides to (they can easily ignore the shellcheck warning, the argument 'but it is so common and always available' is of course moot and opinionated (yes, even busybox features it these days).

The point remains, if you use /bin/sh you kind of imply POSIX compliance, and even if not, some of us would still like this warning to be triggered. Just as which not being standard (again, even with being so ever common).

As with everything, there are exceptions of course. But I think this is not one of them, the exception should be in your code or your parser, where you globally or locally ignore this test as that would work for you. (You being not the individual personal you).

A fair improvement of course would be to the wikipage, for users who run into this, get noted of this.

While surely there are a few valid usecases, I was thinking a bit more about this, and from a users point of view, I can imagine the difference "will this command work in my script (command -v) vs what is the path of this utility (which)". Sometimes a user may be interested only where the binary is, rather then whether it will work. My earlier thoughts still remain the same, but the wiki surly needs clarification of this exceptional case.

Not sure of the complexity, but shellcheck could of course try to figure out the intend. Is the result stored in a variable and that variable only passed along/used as a string, or invoked as a program, it should be a valid use case? Detecting the other uses however is a bit harder I suspect however. but then also relate this to the 'is which being used as a standard tool' or not.

I suppose a decent workaround would be:
WHICH="$(command -v which)"
which would fail if which is not available :) and users of which can use WHICH instead :)

@pedroarthur
Copy link

This is a example where which and command diverge. Using which, I check if all the tools I need exist with a single statement. If I use, command -V, it will return 0 if any of the tools exist.

#!/bin/bash
tooling=( bash no_such_command )

# shellcheck disable=2230
which      "${tooling[@]}" &> /dev/null || echo some tools are missing
command -v "${tooling[@]}" &> /dev/null || echo some tools are missing

Hence, command -v is not a replacement for which.

@oliv3r
Copy link

oliv3r commented Feb 27, 2019

is this even supposed to work?
I use it as such:

check_requirements()
{
    for cmd in ${CMDS}; do
        if ! test_result="$(command -V "${cmd}")"; then
            test_result_fail="${test_result_fail:-}${test_result}\n"
        else
            test_result_pass="${test_result_pass:-}${test_result}\n"
        fi
    done

    echo "Passed tests:"
    # As the results contain \n, we expect these to be interpreted.
    # shellcheck disable=SC2059
    printf "${test_result_pass:-none\n}"
    echo
    echo "Failed tests:"
    # shellcheck disable=SC2059
    printf "${test_result_fail:-none\n}"
    echo

    if [ -n "${test_result_fail:-}" ]; then
        echo "Self-test failed, missing dependencies."
        exit 1
    fi
}

which yes, is a lot longer, but also is a lot friendlier to the user imo :)

so yeah, which can do this in one line, but just because you can, doesn't mean you should imo :)

@pedroarthur
Copy link

is this even supposed to work?

Yes, which is supposed to work this way since day 0. Have you read its man page?

so yeah, which can do this in one line, but just because you can, doesn't mean you should imo :)

So, you advocate to maintain 15+ LoC that triggers far more concerning checks instead of a single one that just has a "non-standard tool" warning? Thank you, but no.

However, the main issue here is saying that things are interchangeable when they aren't. Following the advice in this check can break perfectly working code.

@oliv3r
Copy link

oliv3r commented Feb 28, 2019

is this even supposed to work?

Yes, which is supposed to work this way since day 0. Have you read its man page?
Nope :)

so yeah, which can do this in one line, but just because you can, doesn't mean you should imo :)

So, you advocate to maintain 15+ LoC that triggers far more concerning checks instead of a single one that just has a "non-standard tool" warning? Thank you, but no.
more concerning checks? I personally prefer to have an output like:

Fair point, which does the same in one go, which is indeed (no pun intended) admittedly quite nice. So at the least, the wording could be improved absolutely. But the fact remains, and the warning, that it is a non-standard tool. So a green warning should be sufficient.

However, the main issue here is saying that things are interchangeable when they aren't. Following the advice in this check can break perfectly working code.

nod

@HalisCz
Copy link

HalisCz commented Mar 4, 2019

I've found out another difference between which and command -v:

$ alias ls
alias ls='ls -al'
$ which ls
/usr/bin/ls
$ command -v ls
alias ls='ls -al'
$ type -P ls
/usr/bin/ls

Therefore I think the suggestion should be replace which with type -P.

@ghost
Copy link

ghost commented May 11, 2019

The problem is that type -P doesn't work in some shells that are used as /bin/sh, such as dash. info

For a which (function) that works on bash(/sh), dash and zsh, by using type(optionally with -P if supported) see this gist

PR that added the command -v suggestion: #1123
(linking here so that PR will have a link back to this issue)


one more reason (for me) to not use command -v:

$ sudo touch /usr/bin/mytestfile
[sudo] password for user: 

$ command -v mytestfile
/usr/bin/mytestfile

$ which mytestfile
which: no mytestfile in (/usr/local/sbin:/usr/local/bin:/usr/bin:....blabla)

$ l /usr/bin/mytestfile
-rw-r--r-- 1 root root 0 04.08.2019 22:09 /usr/bin/mytestfile

$ /usr/bin/mytestfile
-bash: /usr/bin/mytestfile: Permission denied

$ command /usr/bin/mytestfile
-bash: /usr/bin/mytestfile: Permission denied

$ bash --version
GNU bash, version 5.0.7(1)-maint (x86_64-pc-linux-gnu)
blah

source: https://unix.stackexchange.com/questions/85249/why-not-use-which-what-to-use-then/85250#comment666687_85250

@leagris
Copy link

leagris commented May 27, 2019

Feel free to use, integrate in your projects, adapt, expand my Bash Implementation of the which command.
It behaves as the native implementation from the debianutils package.

This is the minified version which-min.sh, that is 455 bytes versus 946 bytes for the native version:

#!/usr/bin/env bash
IFS=:;if (($#>0))&&[ "${1:0:1}" == - ];then if [ "$1" != -a ];then printf >&2 'Illegal option %s\nUsage: which [-a] args\n' "$1";exit 2;else b=1;fi;shift;fi;read -rd '' -a g < <(printf %s:\\0 "$PATH");while (($# > 0));do f=0;for d in "${g[@]}";do e="$d/$1";while [[ -L $e && "$(ls -l "$e")" =~ -\>\ (.*) ]];do e="${BASH_REMATCH[1]}";done;if [[ -f $e && -x $e ]];then f=1;echo "$d/$1";((b))||break;fi;done;((f))||c=1;shift;done;exit $c

The human readable non-minified version is here: which.sh.

@abathur
Copy link

abathur commented Jul 12, 2019

I just lost about 40 minutes debugging this one, assuming I'd introduced the error alongside quoting fixes around some alias commands that I made at the same time.

I'm still vaguely glad the issue came up--I realized I could at least swap which for type -P and shave a little execution time off--but it would be nice if the check language directly acknowledged that these aren't fungible replacements.

The current language is quite confident, and I naively took it at face value.

@tripleee
Copy link

tripleee commented Aug 3, 2019

The fundamental problem with which is that it's not reliable. There is a plethora of incompatible implementations, none of which are well-defined or portable. The recommendation should certainly be tweaked to not imply that command -v is always what you want to replace which with; in so many words, we could say "try command -v or type (or perhaps type -p if you are on Bash)".

Here's an excellent background on the topic by the always virtuose @stephane-chazelas:
https://unix.stackexchange.com/a/85250/19240

@Dmole
Copy link

Dmole commented Aug 3, 2019

Shellcheck requires the shell to be specified with #! so non-standard shells can be treated independently of sh/bash.

@AdamDanischewski
Copy link

Shellcheck's web page, states:

ShellCheck
finds bugs in your shell scripts.

It says that it finds bugs it doesn't say anything about portability.

If finding bugs is the goal, then shellcheck should keep quiet about people on Bash using which, type -P, etc - since it is not an error.

Shell check probably needs to beef up its logic to handle differences between major shells - for instance type -P exists reliably in Bash but not in Dash.

Also, I didn't see anyway to specify the shell version - I suppose we should presume a recent version but it would be nice if shellcheck displayed what version of a shell it was running against.

@dseynhae
Copy link

dseynhae commented Oct 4, 2019

I use shellcheck mainly in my Visual Studio Coder extension, where shellcheck issues are neatly listed in the "Problems" console.
So I agree that maybe "Problems" or "Issues" would be a semantically correct term, but IMHO it undervalues the type of feedback we get from shellcheck...

However:

  1. Make the website description more accurate (should be easy).
  2. It might be nice to have:
    "Errors" (bugs)
    "Warnings" (portability issues)
    "Info" (coding style).

@Dmole
Copy link

Dmole commented Oct 4, 2019

  1. It might be nice to have:

In vim shellcheck lists Errors, and Warnings.
info things use the warning tag but start with "Note that".
examples;

error| Don't use $ on the left side of assignments. [SC1066]
warning| X appears unused. Verify it or export it. [SC2034]
warning| Note that, unescaped, this expands on the client side. [SC2029]

@dseynhae
Copy link

dseynhae commented Oct 4, 2019

Indeed! I completely blanked here...
They are even color-coded in Visual Studio Coder:
image

That's what I get for commenting before my morning coffee...

@AdamDanischewski
Copy link

Maybe performance should also be considered:
Screenshot from 2019-10-06 08-46-10

gerhard added a commit to rabbitmq/rabbitmq-server-boshrelease that referenced this issue Jan 23, 2020
It's the right thing to do in this context. See
koalaman/shellcheck#1162 for more info.

Signed-off-by: Gerhard Lazu <gerhard@lazu.co.uk>
@abathur
Copy link

abathur commented Jul 1, 2020

Apparently this check defaults to off since 0f15fa4 was released in 0.7.1.

@EricLuehrsen
Copy link

2. It might be nice to have:
   "Errors" (bugs)
   "Warnings" (portability issues)
   "Info" (coding style).

I tried suggesting a little more lint-like categories a while ago. It would be nice if the numbering scheme followed something like error (1000), warning (2000), info (3000), portability (4000), and best-practice/stye (5000). Deploying shell check as quality tool could be done as many lint-like tools. Over time it would easier to ratchet QA checks on system scripts and supported tools. Both of these ideas were not accepted.

Also /usr/bin/which and unspecific which should have different enable options. The former enabled for "pedantic POSIX" option, and the later normally enabled for naive system administrators. They could even be the same message number but one is "/usr/bin/which may not be portable ..." and the other "which is not portable ..." This is done for a number of GCC diagnostics.

@scop
Copy link
Contributor

scop commented Nov 17, 2021

I wonder if there's a reason not to recommend type -P where it's known to work, both in shellcheck output and at least in wiki? It does work at least with bash, seemingly also with ksh. It was mentioned a couple of times here, the only drawback brought up being portability.

Instead, there are mentions about type -p when on bash in wiki. But given the availability of type -P in bash, I don't know why one would ever want to use type -p in it as a which replacement.

@escoand
Copy link

escoand commented Jun 16, 2022

I've found a somehow portabel solution working on IBM AIX, msys2 and RHEL7.

$ alias grep=true
$ command -v grep
alias grep=true

$ which grep # on IBM AIX
/opt/freeware/bin/grep
$ which grep # on RHEL7
alias grep='true'
        /usr/bin/true 

$ whence grep # on IBM AIX
true
$ whence grep # on RHEL7 and msys2
-bash: whence: command not found

$ type -P grep
/usr/bin/grep
$ type -P grep # on IBM AIX
ksh: whence: 0403-010 A specified flag is not valid for this command.

$ sh -c "command -v grep" # working on all

@abazaba855
Copy link

I've found a somehow portabel solution working on IBM AIX, msys2 and RHEL7.

$ alias grep=true
$ command -v grep
alias grep=true

$ which grep # on IBM AIX
/opt/freeware/bin/grep
$ which grep # on RHEL7
alias grep='true'
        /usr/bin/true 

$ whence grep # on IBM AIX
true
$ whence grep # on RHEL7 and msys2
-bash: whence: command not found

$ type -P grep
/usr/bin/grep
$ type -P grep # on IBM AIX
ksh: whence: 0403-010 A specified flag is not valid for this command.

$ sh -c "command -v grep" # working on all

A minor note on the above: the reason 'whence' appears to only work on IBM AIX, is that 'whence' is a korn shell built-in, and ksh is the default shell on AIX. On RHEL7, the default shell is bash, which does not have the 'whence' built-in. For example, if you were to use ksh on RHEL7, 'whence' would also work. And if you were to use bash on AIX, 'whence' would also fail. The difference is not really OS related, but shell related instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests