control: forwarded -1 https://sourceforge.net/p/clisp/bugs/747/ X-Debbugs-CC: Ariel D'Alessandro <ariel.dalessan...@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.