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