Re: [Quilt-dev] Race in test suite (faildiff.test)

[Jean Delvare]

On Fri, 26 Jan 2018 04:22:09 +0100, Martin Quinson wrote:
> On Thu, Jan 25, 2018 at 11:11:18PM +0100, Jean Delvare wrote:
> > That, on the other hand, works around the problem:
> > 
> > --- a/test/faildiff.test
> > +++ b/test/faildiff.test
> > @@ -27,8 +27,9 @@ What happens on binary files?  
> >     > File test.bin added to patch %{P}test.diff  
> >  
> >     $ printf "\\003\\000\\001" > test.bin
> > -   $ quilt diff -pab --no-index
> > +   $ quilt diff -pab --no-index 2>/dev/null  
> >     >~ (Files|Binary files) a/test\.bin and b/test\.bin differ  
> > +   $ quilt diff -pab --no-index 1>/dev/null  
> >     > Diff failed on file 'test.bin', aborting  
> >     $ echo %{?}  
> >     > 1  
> > 
> > But isn't it more of a hack than an actual fix? Aren't stdout and
> > stderr supposed to be line-buffered? And when pointing to the same file
> > or tty, shouldn't the messages reach said file or tty in the same order
> > as printed in the code?  
> 
> Well, I would say that stdout is line-buffered as long as it's sent to
> a tty, but if it gets onto a pipe it's not line-buffered anymore.

Seems to be the problem indeed. For the purpose of testing I modified
diff_file() in the following way:

--- a/quilt/scripts/patchfns.in
+++ b/quilt/scripts/patchfns.in
@@ -761,7 +761,12 @@ diff_file()
        # Test the return value of diff, and propagate the error retcode if any
        if [ ${PIPESTATUS[0]} == 2 -o ${PIPESTATUS[1]} == 1 ]
        then
-               printf $"Diff failed on file '%s', aborting\n" "$new_file" >&2
+               if [ "$QUILT_COMMAND" = "diff" ]
+               then
+                       printf $"Diff failed on file '%s', aborting\n" 
"$new_file"
+               else
+                       printf $"Diff failed on file '%s', aborting\n" 
"$new_file" >&2
+               fi
                return 1
        fi
 }

That is, if called for a "quilt diff" command, output errors to stdout
instead stderr. With that change, the test suite succeeds 100% of the
time. This confirms that the problem is with the writes being buffered
then flushed in non-deterministic order.

Of course this change is not suitable as a solution as we do want to
direct error messages to stderr.

> But I still don't get why 2>&1 does not helps here.

I see only 2 pipes created between the client and the server in the run
script, one in each direction. It's the client which duplicates one
pipe end to connect it to stderr. Everything from the client back to
the server goes through a single pipe already, so I suspect it's as
if 2>&1 was already passed to all commands.

Anyway, even with 2>&1, stderr and stdout are probably still distinct
file descriptors each with its own pipe buffer, so it does not help?

> I cannot fully
> understand the details of the perl code either, but it seems to me
> like there is too much pipes going on in the exec_test() function. 

I remember looking at that code before and not understanding its
complexity either. At the time I concluded I was not smart enough.

> For example, why are we duplicating STDERR in the client code since
> we're never using the copy? Also, with the following line getting
> executed by the child process right after duplicating STDERR, I don't
> see how pipe redirection at the shell level could have any impact. 
> 
>                 open STDERR, ">&STDOUT"
>               
> But from your tests it does.

Actually no, from my tests it indeed does not.

> This is puzzling.

Sure it is.

-- 
Jean Delvare
SUSE L3 Support