Patchworkβ test: use dtach(1) instead of screen(1) in emacs tests.

login
register
about
Submitter Jameson Graef Rollins
Date 2011-11-10 22:22:17
Message ID <1320963737-1666-1-git-send-email-jrollins@finestructure.net>
Download mbox | patch
Permalink /patch/1483/
State New
Headers show

Comments

Jameson Graef Rollins - 2011-11-10 22:22:17
From: Tomi Ollila <tomi.ollila@iki.fi>

dtach is lighter than screen and is not setuid/setgid program so
TMPDIR does not get reset by dynamic loader when executed.

Signed-off-by: Jameson Graef Rollins <jrollins@finestructure.net>
---
This tweaks the original patch slightly by removing the no-longer needed
screenrc variables.

 test/test-lib.sh |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)
Pieter Praet - 2011-11-11 00:17:37
On Thu, 10 Nov 2011 14:22:17 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> From: Tomi Ollila <tomi.ollila@iki.fi>
> 
> dtach is lighter than screen and is not setuid/setgid program so
> TMPDIR does not get reset by dynamic loader when executed.
> 

`dtach' may be lighter than `screen', but contrary to my expectations,
it appears to be far less efficient:

|      | original [1] | w/ screen [2] | w/ dtach [3] |
|------+--------------+---------------+--------------|
| real |    0m17.570s |     0m20.132s |    0m28.567s |
| user |     0m2.453s |      0m2.593s |     0m2.753s |
| sys  |     0m1.700s |      0m1.577s |     0m1.753s |

> Signed-off-by: Jameson Graef Rollins <jrollins@finestructure.net>
> ---
> This tweaks the original patch slightly by removing the no-longer needed
> screenrc variables.
> 
>  test/test-lib.sh |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/test/test-lib.sh b/test/test-lib.sh
> index c81c709..c0fe037 100755
> --- a/test/test-lib.sh
> +++ b/test/test-lib.sh
> @@ -50,8 +50,6 @@ TZ=UTC
>  TERM=dumb
>  export LANG LC_ALL PAGER TERM TZ
>  GIT_TEST_CMP=${GIT_TEST_CMP:-diff -u}
> -export SCREENRC=/dev/null
> -export SYSSCREENRC=/dev/null
>  
>  # Protect ourselves from common misconfiguration to export
>  # CDPATH into the environment
> @@ -844,7 +842,8 @@ test_emacs () {
>  	if [ -z "$EMACS_SERVER" ]; then
>  		EMACS_SERVER="notmuch-test-suite-$$"
>  		# start a detached screen session with an emacs server
> -		screen -S "$EMACS_SERVER" -d -m "$TMP_DIRECTORY/run_emacs" \
> +		TERM=xterm dtach -n "$TMP_DIRECTORY/dtach-emacs-socket-$$" \

It's worth noting that $TERM is only set to "xterm" to deceive `dtach',
so thankfully, xterm (and all its deps) aren't actually required to run
the tests.

> +			"$TMP_DIRECTORY/run_emacs" \
>  			--no-window-system \
>  			--eval "(setq server-name \"$EMACS_SERVER\")" \
>  			--eval '(server-start)' \
> -- 
> 1.7.7.1
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch


Peace
Tomi Ollila - 2011-11-11 08:09:17
On Fri, 11 Nov 2011 01:17:37 +0100, Pieter Praet <pieter@praet.org> wrote:
> On Thu, 10 Nov 2011 14:22:17 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> > From: Tomi Ollila <tomi.ollila@iki.fi>
> > 
> > dtach is lighter than screen and is not setuid/setgid program so
> > TMPDIR does not get reset by dynamic loader when executed.
> > 
> 
> `dtach' may be lighter than `screen', but contrary to my expectations,
> it appears to be far less efficient:
> 
> |      | original [1] | w/ screen [2] | w/ dtach [3] |
> |------+--------------+---------------+--------------|
> | real |    0m17.570s |     0m20.132s |    0m28.567s |
> | user |     0m2.453s |      0m2.593s |     0m2.753s |
> | sys  |     0m1.700s |      0m1.577s |     0m1.753s |

This sounds interesting; how were these runs measured?
Just using shell builtin time command ?

Does time measure the time of the process and it's subprocesses
or just the process. In any way 20 vs 28 second 'real' time difference
could indicate that there are more other processes running and
the context switch time affects user and system time.

Or, dtach does some stupid io things. Or screen does some advanced
optimizations. Or emacs works more effectively when TERM=screen.
Or the 'virtual' window size emacs sees is unsuitable...


> 
> > Signed-off-by: Jameson Graef Rollins <jrollins@finestructure.net>
> > ---
> > This tweaks the original patch slightly by removing the no-longer needed
> > screenrc variables.
> > 
> >  test/test-lib.sh |    5 ++---
> >  1 files changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/test/test-lib.sh b/test/test-lib.sh
> > index c81c709..c0fe037 100755
> > --- a/test/test-lib.sh
> > +++ b/test/test-lib.sh
> > @@ -50,8 +50,6 @@ TZ=UTC
> >  TERM=dumb
> >  export LANG LC_ALL PAGER TERM TZ
> >  GIT_TEST_CMP=${GIT_TEST_CMP:-diff -u}
> > -export SCREENRC=/dev/null
> > -export SYSSCREENRC=/dev/null
> >  
> >  # Protect ourselves from common misconfiguration to export
> >  # CDPATH into the environment
> > @@ -844,7 +842,8 @@ test_emacs () {
> >  	if [ -z "$EMACS_SERVER" ]; then
> >  		EMACS_SERVER="notmuch-test-suite-$$"
> >  		# start a detached screen session with an emacs server
> > -		screen -S "$EMACS_SERVER" -d -m "$TMP_DIRECTORY/run_emacs" \
> > +		TERM=xterm dtach -n "$TMP_DIRECTORY/dtach-emacs-socket-$$" \
> 
> It's worth noting that $TERM is only set to "xterm" to deceive `dtach',
> so thankfully, xterm (and all its deps) aren't actually required to run
> the tests.

Actually, emacs SIGSEGVs if there is unsuitable TERM in use. I don't know
what it originally was.

> 
> > +			"$TMP_DIRECTORY/run_emacs" \
> >  			--no-window-system \
> >  			--eval "(setq server-name \"$EMACS_SERVER\")" \
> >  			--eval '(server-start)' \
> > -- 
> > 1.7.7.1
> > 
> > _______________________________________________
> > notmuch mailing list
> > notmuch@notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch
> 
> 
> Peace
> 
> -- 
> Pieter
> 
> [1] @ commit 749abb74
> [2] @ commit 760b311b
> [3] @ commit 760b311b + id:"1320963737-1666-1-git-send-email-jrollins@finestructure.net"
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
> 

Tomi
Dmitry Kurochkin - 2011-11-11 08:41:11
Hi Tomi.

On Thu, 10 Nov 2011 14:22:17 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> From: Tomi Ollila <tomi.ollila@iki.fi>
> 
> dtach is lighter than screen and is not setuid/setgid program so
> TMPDIR does not get reset by dynamic loader when executed.
> 
> Signed-off-by: Jameson Graef Rollins <jrollins@finestructure.net>
> ---
> This tweaks the original patch slightly by removing the no-longer needed
> screenrc variables.
> 
>  test/test-lib.sh |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/test/test-lib.sh b/test/test-lib.sh
> index c81c709..c0fe037 100755
> --- a/test/test-lib.sh
> +++ b/test/test-lib.sh
> @@ -50,8 +50,6 @@ TZ=UTC
>  TERM=dumb
>  export LANG LC_ALL PAGER TERM TZ
>  GIT_TEST_CMP=${GIT_TEST_CMP:-diff -u}
> -export SCREENRC=/dev/null
> -export SYSSCREENRC=/dev/null
>  
>  # Protect ourselves from common misconfiguration to export
>  # CDPATH into the environment
> @@ -844,7 +842,8 @@ test_emacs () {
>  	if [ -z "$EMACS_SERVER" ]; then
>  		EMACS_SERVER="notmuch-test-suite-$$"
>  		# start a detached screen session with an emacs server
> -		screen -S "$EMACS_SERVER" -d -m "$TMP_DIRECTORY/run_emacs" \
> +		TERM=xterm dtach -n "$TMP_DIRECTORY/dtach-emacs-socket-$$" \

Can you please give a more detailed explanation (and a comment) to
justify this workaround:

* Is it actually needed for dtach or emacs?  If it is the latter, then
  it should be "dtach ... env TERM=xterm emacs ...", right?

* Why should we care about systems that use terminals without
  corresponding terminfo installed?

* Why is it safe to use xterm?  Is there any standard that requires
  xterm info to be present on the system?

I have tried running dtach with emacs in rxvt-unicode and it worked fine
without overriding TERM.

I have also tried running dtach with invalid TERM:

  $ TERM=non-existing-term dtach -n s /bin/sh
  zsh: can't find terminal definition for non-existing-term

No segfault, shell is started fine.

But emacs does not work without a valid terminfo:

  $ TERM=non-existing-term emacs -nw
  emacs: Terminal type non-existing-term is not defined.
  If that is not the actual type of terminal you have,
  use the Bourne shell command `TERM=... export TERM' (C-shell:
  `setenv TERM ...') to specify the correct type.  It may be necessary
  to do `unset TERMINFO' (C-shell: `unsetenv TERMINFO') as well.

Regards,
  Dmitry

> +			"$TMP_DIRECTORY/run_emacs" \
>  			--no-window-system \
>  			--eval "(setq server-name \"$EMACS_SERVER\")" \
>  			--eval '(server-start)' \
> -- 
> 1.7.7.1
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
Tomi Ollila - 2011-11-11 11:29:48
On Fri, 11 Nov 2011 12:41:11 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Hi Tomi.
> 
> On Thu, 10 Nov 2011 14:22:17 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> > From: Tomi Ollila <tomi.ollila@iki.fi>
> > 
> > dtach is lighter than screen and is not setuid/setgid program so
> > TMPDIR does not get reset by dynamic loader when executed.
> > 
> > Signed-off-by: Jameson Graef Rollins <jrollins@finestructure.net>
> > ---
> > This tweaks the original patch slightly by removing the no-longer needed
> > screenrc variables.
> > 
> >  test/test-lib.sh |    5 ++---
> >  1 files changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/test/test-lib.sh b/test/test-lib.sh
> > index c81c709..c0fe037 100755
> > --- a/test/test-lib.sh
> > +++ b/test/test-lib.sh
> > @@ -50,8 +50,6 @@ TZ=UTC
> >  TERM=dumb
> >  export LANG LC_ALL PAGER TERM TZ
> >  GIT_TEST_CMP=${GIT_TEST_CMP:-diff -u}
> > -export SCREENRC=/dev/null
> > -export SYSSCREENRC=/dev/null
> >  
> >  # Protect ourselves from common misconfiguration to export
> >  # CDPATH into the environment
> > @@ -844,7 +842,8 @@ test_emacs () {
> >  	if [ -z "$EMACS_SERVER" ]; then
> >  		EMACS_SERVER="notmuch-test-suite-$$"
> >  		# start a detached screen session with an emacs server
> > -		screen -S "$EMACS_SERVER" -d -m "$TMP_DIRECTORY/run_emacs" \
> > +		TERM=xterm dtach -n "$TMP_DIRECTORY/dtach-emacs-socket-$$" \
> 
> Can you please give a more detailed explanation (and a comment) to
> justify this workaround:
> 
> * Is it actually needed for dtach or emacs?  If it is the latter, then
>   it should be "dtach ... env TERM=xterm emacs ...", right?

dtach doesn't care about TERM, it is emacs which does (as you noticed
below). But it is bood that the TERM is same in dtach's environment.

doing VAR=val command args instead of env VAR=val saves one execve(2)
as the shell does the environment setting (after fork before execve)
> 
> * Why should we care about systems that use terminals without
>   corresponding terminfo installed?

That's the issue I thought, armdragon on IRC thought etc. The 'xterm'
is part of ncurses-base on Debian, at least (thanks armdragon for this
info) and is it pretty much as base to every terminal emulator there is 
(I just launched gnome-terminal for testing, TERM was set to xterm --
it may have been initially in environment but if gnome-terminal changed
it before launching shell, my shell config will not change it)

> * Why is it safe to use xterm?  Is there any standard that requires
>   xterm info to be present on the system?

xterm is not needed, it is just to tell emacs it can use it's display
control sequences. more common alternative would have been vt100, just
that if one ever wants to attach to that emacs session you he gets
better experience (such as colors), using any terminal emulator, like rxvt.

> I have tried running dtach with emacs in rxvt-unicode and it worked fine
> without overriding TERM.

you had originally suitable TERM to be passed to emacs.

> 
> I have also tried running dtach with invalid TERM:
> 
>   $ TERM=non-existing-term dtach -n s /bin/sh
>   zsh: can't find terminal definition for non-existing-term
> 
> No segfault, shell is started fine.

Yes, dtach could not care less...

> 
> But emacs does not work without a valid terminfo:
> 
>   $ TERM=non-existing-term emacs -nw
>   emacs: Terminal type non-existing-term is not defined.
>   If that is not the actual type of terminal you have,
>   use the Bourne shell command `TERM=... export TERM' (C-shell:
>   `setenv TERM ...') to specify the correct type.  It may be necessary
>   to do `unset TERMINFO' (C-shell: `unsetenv TERMINFO') as well.

Yes, I got SIGSEGV --  strace -o oprfx -ff command args  is your friend :D

> Regards,
>   Dmitry

Thanks for your analysis. 

Tomi

> 
> > +			"$TMP_DIRECTORY/run_emacs" \
> >  			--no-window-system \
> >  			--eval "(setq server-name \"$EMACS_SERVER\")" \
> >  			--eval '(server-start)' \
> > -- 
> > 1.7.7.1
> > 
> > _______________________________________________
> > notmuch mailing list
> > notmuch@notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
>
Dmitry Kurochkin - 2011-11-11 16:02:52
On Fri, 11 Nov 2011 13:29:48 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> On Fri, 11 Nov 2011 12:41:11 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > Hi Tomi.
> > 
> > On Thu, 10 Nov 2011 14:22:17 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> > > From: Tomi Ollila <tomi.ollila@iki.fi>
> > > 
> > > dtach is lighter than screen and is not setuid/setgid program so
> > > TMPDIR does not get reset by dynamic loader when executed.
> > > 
> > > Signed-off-by: Jameson Graef Rollins <jrollins@finestructure.net>
> > > ---
> > > This tweaks the original patch slightly by removing the no-longer needed
> > > screenrc variables.
> > > 
> > >  test/test-lib.sh |    5 ++---
> > >  1 files changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/test/test-lib.sh b/test/test-lib.sh
> > > index c81c709..c0fe037 100755
> > > --- a/test/test-lib.sh
> > > +++ b/test/test-lib.sh
> > > @@ -50,8 +50,6 @@ TZ=UTC
> > >  TERM=dumb
> > >  export LANG LC_ALL PAGER TERM TZ
> > >  GIT_TEST_CMP=${GIT_TEST_CMP:-diff -u}
> > > -export SCREENRC=/dev/null
> > > -export SYSSCREENRC=/dev/null
> > >  
> > >  # Protect ourselves from common misconfiguration to export
> > >  # CDPATH into the environment
> > > @@ -844,7 +842,8 @@ test_emacs () {
> > >  	if [ -z "$EMACS_SERVER" ]; then
> > >  		EMACS_SERVER="notmuch-test-suite-$$"
> > >  		# start a detached screen session with an emacs server
> > > -		screen -S "$EMACS_SERVER" -d -m "$TMP_DIRECTORY/run_emacs" \
> > > +		TERM=xterm dtach -n "$TMP_DIRECTORY/dtach-emacs-socket-$$" \
> > 
> > Can you please give a more detailed explanation (and a comment) to
> > justify this workaround:
> > 
> > * Is it actually needed for dtach or emacs?  If it is the latter, then
> >   it should be "dtach ... env TERM=xterm emacs ...", right?
> 
> dtach doesn't care about TERM, it is emacs which does (as you noticed
> below). But it is bood that the TERM is same in dtach's environment.
> 
> doing VAR=val command args instead of env VAR=val saves one execve(2)
> as the shell does the environment setting (after fork before execve)

I do not think trying that saving one execve(2) call is what we are
after.  Especially in test suite.

IMO setting TERM inside dtach makes it clear that we the workaround is
for emacs, not dtach (a comment should also make this clear).  Besides,
it is always better to minimize the context affected by a hack.

> > 
> > * Why should we care about systems that use terminals without
> >   corresponding terminfo installed?
> 
> That's the issue I thought, armdragon on IRC thought etc. The 'xterm'
> is part of ncurses-base on Debian, at least (thanks armdragon for this
> info) and is it pretty much as base to every terminal emulator there is 
> (I just launched gnome-terminal for testing, TERM was set to xterm --
> it may have been initially in environment but if gnome-terminal changed
> it before launching shell, my shell config will not change it)
> 

Sorry, this does not answer my question.

My point is that if a system can not run "emacs -nw" because it lacks
corresponding terminfo, it is not notmuch who should work around these
issues.  If you are using such system by whatever reason, it is your
responsibility to run notmuch tests (and all other programs who need a
valid terminfo) with modified TERM value.

Perhaps others disagree, but I think it is reasonable for notmuch test
suite to expect that "emacs -nw" works.

> > * Why is it safe to use xterm?  Is there any standard that requires
> >   xterm info to be present on the system?
> 
> xterm is not needed, it is just to tell emacs it can use it's display
> control sequences. more common alternative would have been vt100, just
> that if one ever wants to attach to that emacs session you he gets
> better experience (such as colors), using any terminal emulator, like rxvt.
> 

Sorry, I was misleading.  I did not mean that we are using xterm itself.
I am worried that we introduce dependency on xterm terminfo.  I
understand that it is part of ncurses-base package in Debian.  But
Debian is not the only system out there.  I think it is perfectly fine
for a system without X to lack xterm terminfo.  And by setting TERM to
"xterm" we break such systems.

Using "vt100" seems like a better alternative.  But the question
remains: is there any standard requirement that a particular terminfo
must be available on a system?  If there is no such requirement, then we
can not override TERM value without breaking the test suite on some
weird (but perfectly fine) systems.

> > I have tried running dtach with emacs in rxvt-unicode and it worked fine
> > without overriding TERM.
> 
> you had originally suitable TERM to be passed to emacs.
> 

Sure. Why do not you have a valid TERM value with a corresponding
terminfo on your system?

Sorry for being picky here.  I am fine with your patch except for this.
But TERM override feels wrong to me so far.  (Though, it is not me who
decides on the patches, perhaps others are fine with it and you can just
ignore me.)

Regards,
  Dmitry

> > 
> > I have also tried running dtach with invalid TERM:
> > 
> >   $ TERM=non-existing-term dtach -n s /bin/sh
> >   zsh: can't find terminal definition for non-existing-term
> > 
> > No segfault, shell is started fine.
> 
> Yes, dtach could not care less...
> 
> > 
> > But emacs does not work without a valid terminfo:
> > 
> >   $ TERM=non-existing-term emacs -nw
> >   emacs: Terminal type non-existing-term is not defined.
> >   If that is not the actual type of terminal you have,
> >   use the Bourne shell command `TERM=... export TERM' (C-shell:
> >   `setenv TERM ...') to specify the correct type.  It may be necessary
> >   to do `unset TERMINFO' (C-shell: `unsetenv TERMINFO') as well.
> 
> Yes, I got SIGSEGV --  strace -o oprfx -ff command args  is your friend :D
> 
> > Regards,
> >   Dmitry
> 
> Thanks for your analysis. 
> 
> Tomi
> 
> > 
> > > +			"$TMP_DIRECTORY/run_emacs" \
> > >  			--no-window-system \
> > >  			--eval "(setq server-name \"$EMACS_SERVER\")" \
> > >  			--eval '(server-start)' \
> > > -- 
> > > 1.7.7.1
> > > 
> > > _______________________________________________
> > > notmuch mailing list
> > > notmuch@notmuchmail.org
> > > http://notmuchmail.org/mailman/listinfo/notmuch
> > _______________________________________________
> > notmuch mailing list
> > notmuch@notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch
> >
Jameson Graef Rollins - 2011-11-11 22:51:52
On Fri, 11 Nov 2011 20:02:52 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> My point is that if a system can not run "emacs -nw" because it lacks
> corresponding terminfo, it is not notmuch who should work around these
> issues.  If you are using such system by whatever reason, it is your
> responsibility to run notmuch tests (and all other programs who need a
> valid terminfo) with modified TERM value.
> 
> Perhaps others disagree, but I think it is reasonable for notmuch test
> suite to expect that "emacs -nw" works.

Hi, Dmitry.  I think I disagree with this point.  I think that ideally
the test suite should be able to operate in a very minimal system, even
without a terminal.  Think of automated build daemons.  If the test
suite can provide some sort of terminal emulation for the things that
need it, that would potentially eliminate the need for the runner of the
test to provide it.  It may not be possible to completely abstract
everything away, but to the extent that we can we should.

jamie.

Patch

diff --git a/test/test-lib.sh b/test/test-lib.sh
index c81c709..c0fe037 100755
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -50,8 +50,6 @@  TZ=UTC
 TERM=dumb
 export LANG LC_ALL PAGER TERM TZ
 GIT_TEST_CMP=${GIT_TEST_CMP:-diff -u}
-export SCREENRC=/dev/null
-export SYSSCREENRC=/dev/null
 
 # Protect ourselves from common misconfiguration to export
 # CDPATH into the environment
@@ -844,7 +842,8 @@  test_emacs () {
 	if [ -z "$EMACS_SERVER" ]; then
 		EMACS_SERVER="notmuch-test-suite-$$"
 		# start a detached screen session with an emacs server
-		screen -S "$EMACS_SERVER" -d -m "$TMP_DIRECTORY/run_emacs" \
+		TERM=xterm dtach -n "$TMP_DIRECTORY/dtach-emacs-socket-$$" \
+			"$TMP_DIRECTORY/run_emacs" \
 			--no-window-system \
 			--eval "(setq server-name \"$EMACS_SERVER\")" \
 			--eval '(server-start)' \