Bug#990244: tests/streams: Failing test on pipe stdout file descriptor
FYI, adding +Cc Sam Steingold.
On 12/10/21 4:57 PM, Dennis Filder wrote:
> control: forwarded -1 https://sourceforge.net/p/clisp/bugs/747/
> X-Debbugs-CC: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
>
> On Thu, Dec 09, 2021 at 11:20:52PM -0300, Ariel D'Alessandro wrote:
>> Did a quick check by running the lisp tests using ptywrap and the error
>> is no longer happening. Of course, now the pty is used instead of the
>> (root owned) pipe, so permissions are fine to re-open the file.
>>
>> So, moving forward. This is the failing test simplified:
>>
>> (streamp (setq s (make-stream :error))) T
>> (let ((*reopen-open-file* nil)) ; stdout can be a file, it will be detected!
>> (with-open-file (copy s :direction :output) (streamp copy))) T
>>
>> My question here would be: is this test expected to always succeed?
>
> You'd have to ask the original author of the test that. In a perfect
> world every test would be accompanied by a detailed description of
> what exactly it tests for, what conditions it expects and what
> constitutes a pass or failure.
>
>> AFAIU, we can't assume to have the permissions to re-open the file
>> related to the file descriptor, which is what the code is explicitly
>> trying to do.
>
> In a test suite you usually can since it is probably not supposed to
> be run by a privileged user in such a way that it fails. Build
> servers and CI pipelines, which are almost always involved in exposing
> such bugs, really ought to put in more effort to make their
> environment more similar to the one the developer develops and tests
> the code in instead of silently expecting everyone else to put in the
> additional effort to placate them. Fooling a build/test suite into
> thinking it is hooked to a terminal with a tool like ptywrap is really
> not that big of an ask and it should be a default feature in all such
> products. Alternatively the privileged invoker could just as easily
> call chown(2) on the pipe after opening it and solve the issue this
> way.
>
>> Please correct me if I'm wrong as I'm not used to clisp syntax, just
>> going through the source code :-)
>>
>> From what I see, it responsibility of the clisp test code to check if it
>> has the permissions to re-open the file. Otherwise, it's not guaranteed
>> to succeed. For example, having this fd pointing to a (root owned) pipe
>> is a valid situation and should be properly handled by this test.
>
> That's debatable. Yes, it could be argued that the test should do a
> better job at disarming itself by testing if it can reopen the file
> with e.g. access(2). OTOH it could also be argued that it is the
> invoker's burden to ensure conditions that allow the test to succeed.
> Writing test suites is annoying enough as is. No need to make it more
> annoying by also expecting accommodation for build servers and CI
> pipelines if they themselves could do the accommodation much more
> easily.
>
> Regards.
>
Reply to: