From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
To: Tomi Ollila <tomi.ollila@iki.fi>, Notmuch Mail <notmuch@notmuchmail.org>
Subject: Re: [PATCH] test: use dtach(1) instead of screen(1) in emacs tests.
In-Reply-To: <yf67h36ew2b.fsf@taco2.nixu.fi>
References: <yf6mxc3ab4t.fsf@taco2.nixu.fi>
	<1320963737-1666-1-git-send-email-jrollins@finestructure.net>
	<877h373vbs.fsf@gmail.com> <yf67h36ew2b.fsf@taco2.nixu.fi>
User-Agent: Notmuch/0.9+55~g81c615b (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Fri, 11 Nov 2011 20:02:52 +0400
Message-ID: <87ty6a3avn.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

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
> >