[Pkg-zsh-devel] zsh FTBFS under Salsa CI's reprotest: 'KEY_EVENT' undeclared / blhc CI check disabled / bullseye freeze: zsh is considered a key package

[Daniel Shahaf]

Axel Beckert wrote on Wed, 14 Oct 2020 20:37 +0200:
> Hi Daniel,
> 
> Daniel Shahaf wrote:
> > > P.S.: I wonder if the other, more obscure test suite failure — which
> > > only happens inside the reprotest check on Salsa CI, too — is also
> > > related to eatmydata. Unfortunately the debug output is not as verbose
> > > as in P01privileged.ztst. Like maybe like it is just checked for some
> > > empty output, but it actually contains "ERROR: ld.so: object
> > > 'libeatmydata.so' from LD_PRELOAD cannot be preloaded (cannot open
> > > shared object file): ignored." and the test suite thinks the test
> > > failed because there was any output from some globbing routine.  
> > 
> > Pointer to the "other, more obscure test suite failure", please?  
> 
> Citing from https://salsa.debian.org/debian/zsh/-/jobs/1066149#L6632:
> 
> Running test: unreadable directories can be globbed (users/24619, users/24626)
> Test ../../Test/D02glob.ztst failed: test was expected to fail, but passed.
> Was testing: unreadable directories can be globbed (users/24619, users/24626)
> ../../Test/D02glob.ztst: test failed.
> ../../Test/D03procsubst.ztst: starting.
> 
> See also my bug report against the Salsa CI pipeline:
> https://salsa.debian.org/salsa-ci-team/pipeline/-/issues/181
> 

Oh, I did see both of these.  Sorry, I didn't get the reference.

That test failure can be easily reproduced by adding the 'f' flag to
any existing test case that passes, or by writing a new one such as:

[[[
% git diff
diff --git a/Test/B01cd.ztst b/Test/B01cd.ztst
index bc6757549..daf882ca8 100644
--- a/Test/B01cd.ztst
+++ b/Test/B01cd.ztst
@@ -166,6 +166,9 @@ F:something is broken.  But you already knew that.
 >$mydir/cdtst.tmp/bar
 >$mydir/cdtst.tmp/bar
 
+ :
+0f:this test is expected to fail, but will pass
+
 %clean
 # This optional section cleans up after the test, if necessary,
 # e.g. killing processes etc.  This is in addition to the removal of *.tmp
% make -sj5 all check TESTNUM=B01 
./Test/B01cd.ztst: starting.
Test ./Test/B01cd.ztst was expected to fail, but passed.
Was testing: this test is expected to fail, but will pass
./Test/B01cd.ztst: test XPassed.
**************************************
0 successful test scripts, 1 failure, 0 skipped
**************************************
]]]

(The output is slightly different because I didn't set ZTST_verbose,
and the package build does.)

(In the example the outputs are empty, but the behaviour with non-empty
actual and expected outputs would be the same.)

(I'm looking at upstream master, not at the package.  I haven't checked
whether the test harness was changed upstream since 5.8.)

The test harness doesn't print a diff between the actual and expected
output because the actual output matches the expected output.  That is
implied by the message: "Test ….ztst was expected to fail, but passed".
The expected output is right there in the ztst file, and the actual
output is equal to it, verbatim.  They're not copied into the build
output.

In general, the harness probably _should_ print the actual output if the
expected output is given as patterns («*» at the start of a line) or is
to undergo expansion (the 'q' flag after the expected exit code), and
I think currently it doesn't do so in all cases — which would be an
upstream bug.

Nevertheless, _in the specific case of this test_, since this test
(in 5.8) doesn't use the 'q' flag, doesn't use the «*» output-as-patterns
facility, _and_ doesn't specify the exit code, omitting the actual
output and errput from the error message isn't lossy: the error message
could only have been generated if the actual stdout and stderr were
exactly equal to the expected values, respectively.¹  In particular, if
stderr weren't empty, the test would fail as expected, and thus «make
check» would — assuming all other tests also behave as expected — exit
zero:

[[[
% git diff
diff --git a/Test/B01cd.ztst b/Test/B01cd.ztst
index bc6757549..ed58ec7e2 100644
--- a/Test/B01cd.ztst
+++ b/Test/B01cd.ztst
@@ -166,6 +166,9 @@ F:something is broken.  But you already knew that.
 >$mydir/cdtst.tmp/bar
 >$mydir/cdtst.tmp/bar
 
+ echo foo >&2
+0f:this test is expected to fail, but will pass
+
 %clean
 # This optional section cleans up after the test, if necessary,
 # e.g. killing processes etc.  This is in addition to the removal of *.tmp
                                                                                                                                                                                                                    
% make -sj5 all check TESTNUM=B01 
./Test/B01cd.ztst: starting.
./Test/B01cd.ztst: all tests successful.
**************************************
1 successful test script, 0 failures, 0 skipped
**************************************
% 
]]]

¹ That's also the case for most other XFail tests (see output of
  Test/list-XFails), except one or two tests that use the 'f' flag,
  specify an expected exit code, and specify empty expected output and
  errput.  Those should probably get the 'd' and 'D' flags slapped onto
  them, so if zsh starts to produce the correct exit code _and_
  simultaneously starts to print something to stdout and stderr, the
  behaviour change will still be flagged by the test suite.  This, too,
  is an upstream issue.

So, to summarize:

1. For the failure in question, the actual stdout and stderr are equal
to the expected ones, verbatim.

2. Probable upstream bug: harness should print the actual output when
«*» or «q» are used.

3. Probable upstream bug: Tests marked 0f: or 1f: should get the «d»
and «D» flags.

4. Upstream issue: How to clarify the "was expected to fail, but passed" message?

(Feel free to continue the relevant parts on -workers at .)

> > > P.P.S.: I also wonder if that missing KEY_EVENT thingy could also be
> > > triggered by not being able to load some arbitrary .so file.  
> > 
> > I don't see how, but I suppose it won't hurt to grep «configure &&
> > make»'s log/debug output for eatmydata errors.  
> 
> Yeah, I should probably setup a chroot without eatmydata installed.
> I'm admittedly currently too lazy to do that…

Can't we grep the config.log files that the CI build produces?

> > And note that once the upstream commit you cherry-picked to your branch
> > is merged (whether by being cherry-picked or by packaging 5.9), the
> > failure mode will change: the added «#ifdef KEY_EVENT» will silently
> > evaluate to false, with little consequence.  
> 
> Well, currently for me the main point is that the build no more fails
> with it on Salsa CI, just the test suite. Since I'm not sure if this
> has other implications, I rather used a feature branch for these
> experiments.

Sure, not disagreeing with the use of a branch.  I was just pointing
out that the noisy failure will become silent in the next upstream
version, even if nothing changes on Debian's side.

Cheers,

Daniel