unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* [PATCH] test: redirect STDIN from /dev/tty
@ 2019-05-21 20:17 Tomi Ollila
  2019-05-22  1:24 ` Daniel Kahn Gillmor
  2019-05-22  1:56 ` David Bremner
  0 siblings, 2 replies; 5+ messages in thread
From: Tomi Ollila @ 2019-05-21 20:17 UTC (permalink / raw)
  To: notmuch; +Cc: tomi.ollila

Without this stdin may be anything that parent process provided for it.

Test processes might have tried to read something from it, which would
have caused undeterministic behavior.

E.g. gdb(1) tries to do tty related ioctls on fd 0 (and fd 1 and fd 2,
but those are redirected to 'test.output' before test runs).
---

Hopefully this fixes the parallel problems -- In case of moreutils parallel
only stdout and stderr are captured and all other fd's left untouched
(provided I read web namual correctly). With GNU parallel docs did not
help -- but as we pipe $TESTS to parallel in that case things might be
even more complicated there (i don't undersand why but anyway)

 test/test-lib.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index ed7f6aa7..068e1029 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -55,6 +55,9 @@ done,*)
 	;;
 esac
 
+# STDIN from /dev/null. EOF for readers (and ENOTTY for tty related ioctls).
+exec </dev/null
+
 # Save STDOUT to fd 6 and STDERR to fd 7.
 exec 6>&1 7>&2
 # Make xtrace debugging (when used) use redirected STDERR, with verbose lead:
-- 
2.21.0

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] test: redirect STDIN from /dev/tty
  2019-05-21 20:17 [PATCH] test: redirect STDIN from /dev/tty Tomi Ollila
@ 2019-05-22  1:24 ` Daniel Kahn Gillmor
  2019-05-22  1:56 ` David Bremner
  1 sibling, 0 replies; 5+ messages in thread
From: Daniel Kahn Gillmor @ 2019-05-22  1:24 UTC (permalink / raw)
  To: Tomi Ollila, notmuch

[-- Attachment #1: Type: text/plain, Size: 1977 bytes --]

On Tue 2019-05-21 23:17:02 +0300, Tomi Ollila wrote:
> Without this stdin may be anything that parent process provided for it.

I'm fine with this change -- i can confirm that it avoids the hanging
problem on debian stable for me.

please merge either this, or
id:20190521010304.417-1-dkg@fifthhorseman.net . i don't think we need
both.

Tomi's step here is a "big hammer" approach by comparison with my patch
targeting gdb itself, but it's also simple and elegant -- stdin from
outside the test suite has no business interfering with the tests.

> Test processes might have tried to read something from it, which would
> have caused undeterministic behavior.

My only (weak, nagging) concern is that this change may err on the side
of "too much determinism", in the sense that different stdin setups
might more accurately represent "real world" use cases of notmuch than
stdin being mapped to /dev/null.

However, if those different configurations that we happen to get from
random people invoking the test suite in different ways are actually
important, then the test suite should probably try to explicitly
enumerate those cases, and test them regardless of the environment in
which the test suite is run.

> E.g. gdb(1) tries to do tty related ioctls on fd 0 (and fd 1 and fd 2,
> but those are redirected to 'test.output' before test runs).

For the record, these problems were only with moreutils parallel when
used in combination with a specific version of gdb.

if gdb (or some other subprocess (gpg, i'm looking at you)) were to try
to monkey around with /dev/tty, i think even with this "big
hammer" fix, it could do so, because we haven't isolated the tests from
the "controlling terminal" (see tty(4)).

But again, if we run into that, that is probably worth an independent
fix (with "setsid --wait" or something like that, ugh).

        --dkg

PS if we merge this change, does it mean that we can/should remove the
   -tty /dev/null stuff from T380-atomicity.sh?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] test: redirect STDIN from /dev/tty
  2019-05-21 20:17 [PATCH] test: redirect STDIN from /dev/tty Tomi Ollila
  2019-05-22  1:24 ` Daniel Kahn Gillmor
@ 2019-05-22  1:56 ` David Bremner
  2019-05-22  5:29   ` Tomi Ollila
  1 sibling, 1 reply; 5+ messages in thread
From: David Bremner @ 2019-05-22  1:56 UTC (permalink / raw)
  To: Tomi Ollila, notmuch; +Cc: tomi.ollila

Tomi Ollila <tomi.ollila@iki.fi> writes:

> Without this stdin may be anything that parent process provided for it.
>
> Test processes might have tried to read something from it, which would
> have caused undeterministic behavior.
>
> E.g. gdb(1) tries to do tty related ioctls on fd 0 (and fd 1 and fd 2,
> but those are redirected to 'test.output' before test runs).
> ---
>
> Hopefully this fixes the parallel problems -- In case of moreutils parallel
> only stdout and stderr are captured and all other fd's left untouched
> (provided I read web namual correctly). With GNU parallel docs did not
> help -- but as we pipe $TESTS to parallel in that case things might be
> even more complicated there (i don't undersand why but anyway)

I can confirm that this seems to fix the parallel test problems with
moreutils parallel and GNU.

I think I'm leaning towards this fix between the two, but I'll sleep on
it and decide tommorow.

d

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] test: redirect STDIN from /dev/tty
  2019-05-22  1:56 ` David Bremner
@ 2019-05-22  5:29   ` Tomi Ollila
  2019-05-22 11:49     ` David Bremner
  0 siblings, 1 reply; 5+ messages in thread
From: Tomi Ollila @ 2019-05-22  5:29 UTC (permalink / raw)
  To: David Bremner, notmuch

On Tue, May 21 2019, David Bremner wrote:

> Tomi Ollila <tomi.ollila@iki.fi> writes:
>
>> Without this stdin may be anything that parent process provided for it.
>>
>> Test processes might have tried to read something from it, which would
>> have caused undeterministic behavior.
>>
>> E.g. gdb(1) tries to do tty related ioctls on fd 0 (and fd 1 and fd 2,
>> but those are redirected to 'test.output' before test runs).
>> ---
>>
>> Hopefully this fixes the parallel problems -- In case of moreutils parallel
>> only stdout and stderr are captured and all other fd's left untouched
>> (provided I read web namual correctly). With GNU parallel docs did not
>> help -- but as we pipe $TESTS to parallel in that case things might be
>> even more complicated there (i don't undersand why but anyway)
>
> I can confirm that this seems to fix the parallel test problems with
> moreutils parallel and GNU.
>
> I think I'm leaning towards this fix between the two, but I'll sleep on
> it and decide tommorow.

... and when you merge this one, please amend /dev/tty -> /dev/null 
--- in subject line of the commit message ;D

Tomi

>
> d
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] test: redirect STDIN from /dev/tty
  2019-05-22  5:29   ` Tomi Ollila
@ 2019-05-22 11:49     ` David Bremner
  0 siblings, 0 replies; 5+ messages in thread
From: David Bremner @ 2019-05-22 11:49 UTC (permalink / raw)
  To: Tomi Ollila, notmuch

Tomi Ollila <tomi.ollila@iki.fi> writes:

> On Tue, May 21 2019, David Bremner wrote:
>
>> Tomi Ollila <tomi.ollila@iki.fi> writes:
>>
>>> Without this stdin may be anything that parent process provided for it.
>>>
>>> Test processes might have tried to read something from it, which would
>>> have caused undeterministic behavior.
>>>
>>> E.g. gdb(1) tries to do tty related ioctls on fd 0 (and fd 1 and fd 2,
>>> but those are redirected to 'test.output' before test runs).
>>> ---
>>>
>>> Hopefully this fixes the parallel problems -- In case of moreutils parallel
>>> only stdout and stderr are captured and all other fd's left untouched
>>> (provided I read web namual correctly). With GNU parallel docs did not
>>> help -- but as we pipe $TESTS to parallel in that case things might be
>>> even more complicated there (i don't undersand why but anyway)
>>
>> I can confirm that this seems to fix the parallel test problems with
>> moreutils parallel and GNU.
>>
>> I think I'm leaning towards this fix between the two, but I'll sleep on
>> it and decide tommorow.
>
> ... and when you merge this one, please amend /dev/tty -> /dev/null 
> --- in subject line of the commit message ;D

Done, and pushed.

d

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-05-22 11:49 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-05-21 20:17 [PATCH] test: redirect STDIN from /dev/tty Tomi Ollila
2019-05-22  1:24 ` Daniel Kahn Gillmor
2019-05-22  1:56 ` David Bremner
2019-05-22  5:29   ` Tomi Ollila
2019-05-22 11:49     ` David Bremner

Code repositories for project(s) associated with this public inbox

	https://yhetil.org/notmuch.git/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).