Control: severity -1 normal
Control: tag -1 - patch
Dear Gianfranco,
first thanks for making me aware of suboptimal upstream code.
Gianfranco Costamagna wrote:
> Hello, looks like your dkms ship file is sourcing external stuff
> with pushd/popd and so on.
Not "external" stuff but stuff from its own source.
> I don't think this is a sane idea,
I disagree. Using pushd/popd (or "cd -") in scripts is a valid way to
avoid a helper variable to save the previous directory.
But what I dislike in the aforementioned code snippet is that
$BASH_SOURCE is
a) used at all and hence the script is not bourne-shell compatible.
b) is used in an unclear way (couldn't find in bash(1) what the
meaning of using an array as scalar should return. From my
experiments, it seems to return its first value.)
I also disagree that "I don't think this is a sane idea" is RC-level,
not even important. Hence downgrading to "normal.
> specially because in Debian we don't even use version.sh script to
> fill the dkms.conf file.
I don't understand what you refer to with "in Debian". Do you mean the
fact that I didn't ship the package's upstream's version.sh? Do you
think I should?
> Can you please remove the two lines?
At least not in the way you propsed. Hence removing the tag "patch".
> this is what we do to test dkms packages:
[...]
> dkms_pkg=$(bash -c ". $dkms_conf; echo \$PACKAGE_NAME" 2>/dev/null)
> dkms_ver=$(bash -c ". $dkms_conf; echo \$PACKAGE_VERSION" 2>/dev/null)
You could do ". $dkms_conf > /dev/null"
> bash -c ". dkms.conf; echo \$PACKAGE_NAME"
> /tmp/iptables-netflow-2.5 /tmp/iptables-netflow-2.5
> /tmp/iptables-netflow-2.5
> ipt-netflow
JFTR: This can also happen with "cd", depending on the shell's
settings. (IIRC setting CDPATH might cause this. Granted, "bash -c" is
probably neither an interactive nor an login shell and hence shouldn't
source any local rc files, but I've ran into this with "cd" and some
non-DKMS scripts, too, on some machines not administrated by myself.
> I think dkms.conf files are meant to be sourced from outside, and
> launching scripts is a bad idea.
I don't see how this relates.
> --- iptables-netflow-2.5.orig/dkms.conf
> +++ iptables-netflow-2.5/dkms.conf
> @@ -1,7 +1,5 @@
> PACKAGE_NAME="ipt-netflow"
> -pushd `dirname $BASH_SOURCE`
> PACKAGE_VERSION=`./version.sh`
> -popd
IMHO this makes it worse, because it will source version.sh from
whatever working directory the script that sources dkms.conf was
called from.
I see these ways to solve this:
* Remove that whole block including "PACKAGE_VERSION=`./version.sh`",
or
* make pushd/popd silent
Regards, Axel
--
,''`. | Axel Beckert <a...@debian.org>, https://people.debian.org/~abe/
: :' : | Debian Developer, ftp.ch.debian.org Admin
`. `' | 4096R: 2517 B724 C5F6 CA99 5329 6E61 2FF9 CD59 6126 16B5
`- | 1024D: F067 EA27 26B9 C3FC 1486 202E C09E 1D89 9593 0EDE
