Bug#966483: iptables-netflow: sourcing of external scripts in dkms file?
Control: tag -1 - confirmed + moreinfo Hi Gianfranco, now that Bullseye is released and I'm preparing the upload of a new upstream release, I've revisited this bug report. Gianfranco Costamagna wrote: > > Yeah, and the version.sh call itself can be removed, too. Will do. Planned to do that(*), but … > Unfortunately, it became a little difficult to implement, due to the > version.sh file > not being easy to convert into a "sed s/FOO/foo/g < dkms.in > dkms" > > This is what I did before thinking it was not easily upstreamable: … now you've lost me complete. Was that meant as patch to apply to the package? Or was it just a historical digression? If this was meant as to be included, I assume that file name was still preliminary: > --- iptables-netflow-2.5/debian/patches/patch 1970-01-01 01:00:00.0 > +0100 > +++ iptables-netflow-2.5/debian/patches/patch 2020-08-11 21:00:13.0 > +0200 And the entry in debian/patches/series seems missing, too. > Feel free to send it upstream if you like it! Admittedly it looks more complicated than necessary. And despite having reread the whole bug report at least twice, I'm no more seeing the motivation for this, also because dkms itself has been fixed by you. Actually I'm not even seeing the motivation to remove that popd/pushd/version.sh part anymore. Sorry. Regards, Axel -- ,''`. | Axel Beckert , 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
Bug#966483: iptables-netflow: sourcing of external scripts in dkms file?
Hello, On Tue, 11 Aug 2020 21:06:02 +0200 Gianfranco Costamagna wrote: > Hello Axel, > > > Yeah, and the version.sh call itself can be removed, too. Will do. > > > > Thanks for bringing this up despite the initially differing opinions. > > :-) > > > > thanks to you! > > And forgive me if I wasn't clear enough, I wrote the email after a somewhat > deep analysis of the issue, and my initial effort was to have something like > a > "dkms.in" evaulated during build into a "dkms" file, after doing the > substitutions. > > Unfortunately, it became a little difficult to implement, due to the > version.sh file > not being easy to convert into a "sed s/FOO/foo/g < dkms.in > dkms" > > This is what I did before thinking it was not easily upstreamable: > > diff -Nru iptables-netflow-2.5/debian/patches/patch > iptables-netflow-2.5/debian/patches/patch > --- iptables-netflow-2.5/debian/patches/patch 1970-01-01 01:00:00.0 > +0100 > +++ iptables-netflow-2.5/debian/patches/patch 2020-08-11 21:00:13.0 > +0200 > @@ -0,0 +1,30 @@ > +--- iptables-netflow-2.5.orig/Makefile.in > iptables-netflow-2.5/Makefile.in > +@@ -43,7 +43,7 @@ mclean: > + lclean: > + -rm -f *.so *_sh.o > + clean: mclean lclean > +--rm -f *.so *.o modules.order version.h compat_def.h > ++-rm -f *.so *.o modules.order version.h compat_def.h dkms.conf > + > + snmp_NETFLOW.so: snmp_NETFLOW.c > + $(CC) -fPIC -shared -o $@ $< -lnetsnmp > +@@ -76,6 +76,7 @@ version.h: ipt_NETFLOW.c ipt_NETFLOW.h c > + > + linstall: | libipt_NETFLOW.so libip6t_NETFLOW.so > + @echo " *" > ++sed s/@VERSION@/$(shell ./version.sh)/g < dkms.conf.in > dkms.conf > + install -D libipt_NETFLOW.so > $(DESTDIR)$(IPTABLES_MODULES)/libipt_NETFLOW.so > + install -D libip6t_NETFLOW.so > $(DESTDIR)$(IPTABLES_MODULES)/libip6t_NETFLOW.so > + > +--- /dev/null > iptables-netflow-2.5/dkms.conf.in > +@@ -0,0 +1,8 @@ > ++PACKAGE_NAME="ipt-netflow" > ++PACKAGE_VERSION=@VERSION@ > ++BUILT_MODULE_NAME[0]=ipt_NETFLOW > ++DEST_MODULE_LOCATION[0]=/kernel/extra > ++STRIP[0]=no > ++MAKE[0]="make ipt_NETFLOW.ko" > ++PRE_BUILD="./configure --from-dkms-conf=$kernel_source_dir" > ++AUTOINSTALL=yes > > > and then I simplified rules file, to drop the VERSION sed. > > --- iptables-netflow-2.5/debian/rules 2020-05-20 17:20:43.0 +0200 > +++ iptables-netflow-2.5/debian/rules 2020-08-11 21:01:54.0 +0200 > @@ -42,5 +42,5 @@ ping? G.
Bug#966483: iptables-netflow: sourcing of external scripts in dkms file?
Hello Axel, > Yeah, and the version.sh call itself can be removed, too. Will do. > > Thanks for bringing this up despite the initially differing opinions. > :-) > thanks to you! And forgive me if I wasn't clear enough, I wrote the email after a somewhat deep analysis of the issue, and my initial effort was to have something like a "dkms.in" evaulated during build into a "dkms" file, after doing the substitutions. Unfortunately, it became a little difficult to implement, due to the version.sh file not being easy to convert into a "sed s/FOO/foo/g < dkms.in > dkms" This is what I did before thinking it was not easily upstreamable: diff -Nru iptables-netflow-2.5/debian/patches/patch iptables-netflow-2.5/debian/patches/patch --- iptables-netflow-2.5/debian/patches/patch 1970-01-01 01:00:00.0 +0100 +++ iptables-netflow-2.5/debian/patches/patch 2020-08-11 21:00:13.0 +0200 @@ -0,0 +1,30 @@ +--- iptables-netflow-2.5.orig/Makefile.in iptables-netflow-2.5/Makefile.in +@@ -43,7 +43,7 @@ mclean: + lclean: + -rm -f *.so *_sh.o + clean: mclean lclean +- -rm -f *.so *.o modules.order version.h compat_def.h ++ -rm -f *.so *.o modules.order version.h compat_def.h dkms.conf + + snmp_NETFLOW.so: snmp_NETFLOW.c + $(CC) -fPIC -shared -o $@ $< -lnetsnmp +@@ -76,6 +76,7 @@ version.h: ipt_NETFLOW.c ipt_NETFLOW.h c + + linstall: | libipt_NETFLOW.so libip6t_NETFLOW.so + @echo " *" ++ sed s/@VERSION@/$(shell ./version.sh)/g < dkms.conf.in > dkms.conf + install -D libipt_NETFLOW.so $(DESTDIR)$(IPTABLES_MODULES)/libipt_NETFLOW.so + install -D libip6t_NETFLOW.so $(DESTDIR)$(IPTABLES_MODULES)/libip6t_NETFLOW.so + +--- /dev/null iptables-netflow-2.5/dkms.conf.in +@@ -0,0 +1,8 @@ ++PACKAGE_NAME="ipt-netflow" ++PACKAGE_VERSION=@VERSION@ ++BUILT_MODULE_NAME[0]=ipt_NETFLOW ++DEST_MODULE_LOCATION[0]=/kernel/extra ++STRIP[0]=no ++MAKE[0]="make ipt_NETFLOW.ko" ++PRE_BUILD="./configure --from-dkms-conf=$kernel_source_dir" ++AUTOINSTALL=yes and then I simplified rules file, to drop the VERSION sed. --- iptables-netflow-2.5/debian/rules 2020-05-20 17:20:43.0 +0200 +++ iptables-netflow-2.5/debian/rules 2020-08-11 21:01:54.0 +0200 @@ -42,5 +42,5 @@ [ ! -e Makefile ] || $(MAKE) lclean override_dh_dkms: - sed -e 's#`\./version.sh`#$(DEB_VERSION_UPSTREAM)#;s#^PRE_BUILD="\(.*\)"#PRE_BUILD="\1 $(DKMS_CONFIGURE_OPTIONS)"#' dkms.conf > debian/dkms + sed -e 's#^PRE_BUILD="\(.*\)"#PRE_BUILD="\1 $(DKMS_CONFIGURE_OPTIONS)"#' dkms.conf > debian/dkms dh_dkms this looks somewhat upstreamable, and also Debian friendly (I'm not sure if having a .git directory makes the script fail to provide the correct output, this is one reason for me not giving the patch before) Feel free to send it upstream if you like it! thanks a lot! G.
Bug#966483: iptables-netflow: sourcing of external scripts in dkms file?
Dear Gianfranco, Gianfranco Costamagna wrote: > > > 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? > > I think we shouldn't, because it is used/useful only at build time... Thanks for your comment on this! > > > 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" > > interesting, this works indeed: [...] > (and uploaded in sid) Yay! :-) > Honestly, I still think my patch is something sane to do (of course, as > Debian specific patch), because of this done in rules file: > override_dh_dkms: > sed -e > 's#`\./version.sh`#$(DEB_VERSION_UPSTREAM)#;s#^PRE_BUILD="\(.*\)"#PRE_BUILD="\1 > $(DKMS_CONFIGURE_OPTIONS)"#' dkms.conf > debian/dkms > dh_dkms > > so, in any case, that version.sh is *never* ran in Debian packaging, > so the whole pushd/popd are useless in this context. Yeah, and the version.sh call itself can be removed, too. Will do. Thanks for bringing this up despite the initially differing opinions. :-) Regards, Axel -- ,''`. | Axel Beckert , 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
Bug#966483: iptables-netflow: sourcing of external scripts in dkms file?
Hello! > Not "external" stuff but stuff from its own source. > oops, you are right... > > 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. > right also here > > 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? I think we shouldn't, because it is used/useful only at build time... > > > 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" > interesting, this works indeed: +dkms_pkg=$(bash -c ". $dkms_conf > /dev/null; echo \$PACKAGE_NAME" 2>/dev/null) +dkms_ver=$(bash -c ". $dkms_conf > /dev/null; echo \$PACKAGE_VERSION" 2>/dev/null) (and uploaded in sid) Honestly, I still think my patch is something sane to do (of course, as Debian specific patch), because of this done in rules file: override_dh_dkms: sed -e 's#`\./version.sh`#$(DEB_VERSION_UPSTREAM)#;s#^PRE_BUILD="\(.*\)"#PRE_BUILD="\1 $(DKMS_CONFIGURE_OPTIONS)"#' dkms.conf > debian/dkms dh_dkms so, in any case, that version.sh is *never* ran in Debian packaging, so the whole pushd/popd are useless in this context. G.
Bug#966483: iptables-netflow: sourcing of external scripts in dkms file?
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 , 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
Bug#966483: iptables-netflow: sourcing of external scripts in dkms file?
Source: iptables-netflow Version: 2.5-2 Severity: serious tags: patch Hello, looks like your dkms ship file is sourcing external stuff with pushd/popd and so on. I don't think this is a sane idea, specially because in Debian we don't even use version.sh script to fill the dkms.conf file. Can you please remove the two lines? this is what we do to test dkms packages: if ! dkms_conf=$(dpkg -L $pkg | grep '/usr/src' | grep '/dkms.conf$'); then echo "I: Package $pkg has no dkms.conf, skipping" return fi echo "I: Testing binary package $pkg" dkms_pkg=$(bash -c ". $dkms_conf; echo \$PACKAGE_NAME" 2>/dev/null) dkms_ver=$(bash -c ". $dkms_conf; echo \$PACKAGE_VERSION" 2>/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 I think dkms.conf files are meant to be sourced from outside, and launching scripts is a bad idea. this is the trivial patch: Description: Don't print useless stuff and change directory in dkms file, it is meant to be sourced from outside. Author: Gianfranco Costamagna Bug-Debian: https://bugs.debian.org/-1 Last-Update: 2020-07-29 --- 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 BUILT_MODULE_NAME[0]=ipt_NETFLOW DEST_MODULE_LOCATION[0]=/kernel/extra STRIP[0]=no
