Bug#1001677: lintian: check for: "cd ... py3versions -r" in autopkgtest scripts
Hi Mattia,
On Tue, Dec 14, 2021 at 03:39:54PM +0100, Mattia Rizzolo wrote:
> Hi Julian,
>
> On Tue, Dec 14, 2021 at 06:49:12AM +0000, Julian Gilbey wrote:
> > I discovered that in several of my autopkgtest scripts, and in various
> > other packages in the archive, the following pattern appears:
>
> I think (although I'm not an authoritative voice of any kind here) that
> you you are using the wrong option altogether actually.
That may or may not be true. What I am observing, though, is that
there are a number of packages that have this bug. Not many, but a
moderate number nonetheless.
> > cd somewhere
> > ...
> > for py in $(py3versions -r 2>/dev/null)
> > ...
> >
> > Unfortunately, this silently fails,
>
> Of course it's silent. you are asking something and then ignoring the
> output…
Well, yes and no. In a package that doesn't have X-Python3-Version
set, py3versions -r prints a warning message to stderr, and we want to
suppress this so that autopkgtest doesn't fail for no reason (adding
Restrictions: allow-stderr for a single expected warning is not a good
thing to do). However, this doesn't separate from the case where
py3versions -r prints an error message and exits with an error status.
> > is given. The "2>/dev/null" is nevertheless usually required even
> > when run from the correct directory to silence the warning:
> >
> > py3versions: no X-Python3-Version in control file, using supported versions
>
> that's because you are using the wrong option.
You may well be correct. That doesn't change the reality that this is
what is happening in a number of packages.
> You should use `-s` instead of `-r` in those cases, and drop the
> 2>/dev/null.
I'm not convinced; if a X-Python3-Version is added later to a package
using -s, then things might go wrong, whereas -r will always do the
right thing as it falls back to -s. Or if copying boilerplate testing
code, -r always works but -s doesn't. A better possibility might be
for py3versions to introduce an additional option -q/--quiet to
suppress the warning message when -r falls back to -s; this is vastly
superior to 2>/dev/null as it will let the true failure remain
visible (and possibly cause autopkgtest to correctly fail).
> Besides, even if you keep the -r it wouldn't be much of a problem: $()
> only captures stdout, stderr just gets printed and doesn't interfere
> with the for loop or such, so why are you doing this?
Because any output to stderr causes autopkgtest to fail unless it is
specifically instructed to allow-stderr.
> -r is used by dh_python and other build scripts because they have to
> support all packages, but IMHO you really should be using -s if you know
> you don't have X-Python3-Version in your package and you *really* want
> to just suppose whatever are the current supported python3 versions.
You may indeed be right.
> > The corrected script should read something like:
> >
> > ...
> > for py in $(py3versions -r 2>/dev/null); do
> > cd somewhere
> > ...
>
>
> And then, `py3versions -s` will "just work" wherever you are, no need to
> do this fairly complicated check.
True.
> > A regex such as /\bcd\b.*py3versions\s+-r/s applied to the entire
> > content of debian/tests/control and every other file in debian/tests
> > should catch this issue.
>
> Yeah sure, and then what about pushd ? Doing this kind of check in
> lintian is fraught with false positives, so I recommend the lintian
> maintainers don't try to do this.
I don't think I've yet found a case where pushd has been incorrectly
used (and I've so far checked all source packages beginning with a-f
and all source packages with "python" in their name).
> However, instead, I'd suggest that, after checking with the
> debian-python@ lists, we could tell people to use -s if and only if
> X-Python3-Version is not defined (conversely, we should warn if packages
> use -s if X-Python3-Version *is* defined, probably).
That sounds very sensible. If people hear the advice. And how does
one check that the advice is being followed? Hence my suggestion to
have a lintian check for this.
But it may be a moot point: if it turns out that only a handful of
packages have this issue (and I'm currently filing bug reports on
them), then once they are fixed, maintainers who copy such scripts are
unlikely to reintroduce the issue.
Best wishes,
Julian
Reply to: