Bug#883742: developers-reference: §6.4: Please don't recommend to use "if [ -x /usr/bin/<command> ]; …" in maintainer scripts

[Axel Beckert <abe@debian.org>]

Package: developers-reference
Version: 3.4.19
Severity: normal
Tags: patch

Hi,

§6.4 recommends:
> If you need to check for the existence of a command, you should use
> something like
>
> if [ -x /usr/sbin/install-docs ]; then ...

This has been proven to introduce technical debt (see
https://lists.debian.org/debian-devel/2014/11/msg00044.html and
https://bugs.debian.org/769845) if any of those paths is ever changed
like e.g. moved from /usr/sbin/ to /usr/bin/.

Since #769845 has been fixed, Lintian even emits a warning if such a
case is found.

Bascially there are two cases of "if [ -x /usr/bin/<command> ]; …" in
maintainer scripts:

A) The maintainer controls the checked path (and doesn't forget to
   update the maintainer script in case he ever changes the program's
   path).

B) The maintainer doesn't control the checked path. (See #769845 and
   https://lists.debian.org/debian-devel/2014/11/msg00044.html for real
   examples.)

Overriding this tag in case (A) is ok-ish if the maintainer indeed
doesn't forget to also update the maintainer scripts if he changes the
path.

But case (B) is a real issue and should be fixed and not overridden,
because the maintainer script definitely behaves worse if the wrong path
(e.g. a path only valid in earlier Debian releases) is used in the
check.

So in general, using "if [ -x /usr/bin/<command> ]; …" in maintainer
scripts should be avoided and definitely not recommended.

Leaves the question what should be recommended instead.

According to the discussion in https://bugs.debian.org/807695 the
current recommendation of a 12 lines shell script is unacceptable for
those developers who took part in the discussion and a simple "if which
$command > /dev/null; …" should be used instead.

(Note: Using "type" instead of "which" seems disputed or at least
unclear due to differences between bash and dash. See
https://bugs.debian.org/747320.)

So my suggestion is the following patch:

diff --git a/best-pkging-practices.dbk b/best-pkging-practices.dbk
index 27b5eca..51617ec 100644
--- a/best-pkging-practices.dbk
+++ b/best-pkging-practices.dbk
@@ -659,12 +659,7 @@ maintainer script.
 If you need to check for the existence of a command, you should use something
 like
 </para>
-<programlisting>if [ -x /usr/sbin/install-docs ]; then ...</programlisting>
-<para>
-If you don't wish to hard-code the path of a command in your maintainer script,
-the following POSIX-compliant shell function may help:
-</para>
-&example-pathfind;
+<programlisting>if which install-docs > /dev/null; then ...</programlisting>
 <para>
 You can use this function to search <varname>$PATH</varname> for a command
 name, passed as an argument.  It returns true (zero) if the command was found,
diff --git a/common.ent b/common.ent
index 1bbed6a..66e0e58 100644
--- a/common.ent
+++ b/common.ent
@@ -256,16 +256,3 @@ pool/non-free/f/firmware-nonfree/
 
 uudecode-file:
     perl -ne 'print(unpack &quot;u&quot;, $$_);' $(file).uuencoded &gt; $(file)</programlisting>">
-
-<!ENTITY example-pathfind '<programlisting>pathfind() {
-    OLDIFS="$IFS"
-    IFS=:
-    for p in $PATH; do
-        if [ -x "$p/$*" ]; then
-            IFS="$OLDIFS"
-            return 0
-        fi
-    done
-    IFS="$OLDIFS"
-    return 1
-}</programlisting>'>

I've pushed that patch into a new branch named "devref-vs-#769845" in
the git repository:

https://anonscm.debian.org/cgit/collab-maint/developers-reference.git/log/?h=devref-vs-%23769845

Feel free to continue to work on the fix for this bug in that branch
until there is a consent on merging the branch.

P.S.: I've taken the same severity as used in #769845 since I consider
this bug report equivalent to #769845, just against a different package.

-- System Information:
Debian Release: buster/sid
  APT prefers unstable
  APT policy: (990, 'unstable'), (600, 'testing'), (500, 'unstable-debug'), (500, 'buildd-unstable'), (110, 'experimental'), (1, 'experimental-debug'), (1, 'buildd-experimental')
Architecture: amd64 (x86_64)

Kernel: Linux 4.13.0-rc7-amd64 (SMP w/8 CPU cores)
Locale: LANG=C.UTF-8, LC_CTYPE=C.UTF-8 (charmap=UTF-8), LANGUAGE=C.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
Init: sysvinit (via /sbin/init)

developers-reference depends on no packages.

Versions of packages developers-reference recommends:
ii  debian-policy  4.1.2.0

Versions of packages developers-reference suggests:
ii  doc-base  0.10.7

-- no debconf information