Bug 10351

Summary: Use a login bash shell to execute commands started from the tray menu
Product: cygwin Reporter: Jon TURNEY <jon.turney>
Component: Cygwin/XAssignee: Yaakov Selkowitz <yselkowi>
Status: RESOLVED FIXED    
Severity: normal    
Priority: P2    
Version: unspecified   
Target Milestone: ---   
Host: Target:
Build: Last reconfirmed:
Attachments: Patch to use login bash shell for tray menu commands
patch for ports/xorg/xinit
Tidy ups for system.Xwinrc

Description Jon TURNEY 2009-06-30 16:58:10 UTC
Following discussion at http://cygwin.com/ml/cygwin-xfree/2009-06/msg00010.html,
change to using a login bash shell to execute the commands started from the tray
menu
Comment 1 Jon TURNEY 2009-06-30 16:58:57 UTC
Created attachment 4031 [details]
Patch to use login bash shell for tray menu commands
Comment 2 Yaakov Selkowitz 2009-07-02 18:08:19 UTC
Thanks; patch added to the queue for the next release.
Comment 3 Yaakov Selkowitz 2009-07-08 23:29:50 UTC
Shipped in 1.6.2-1; closing.
Comment 4 Yaakov Selkowitz 2009-09-04 08:23:21 UTC
Trying the tray menu for the first time in a while, I see that using a login
shell is expensive in terms of launch times.

If I understand the thread correctly, the problem is only if XWin is launched
directly from Windows.  Maybe startxwin.{bat,sh} should launch XWin via "bash -l
-c" instead, so the environment initialization need occur only once?
Comment 5 Yaakov Selkowitz 2009-09-30 17:22:09 UTC
Created attachment 4237 [details]
patch for ports/xorg/xinit

Here is what I am thinking for this bug and bug 10598.	Can we use this
instead, and revert the patch to xserver?
Comment 6 Jon TURNEY 2009-09-30 20:24:23 UTC
(In reply to comment #5)
> Created an attachment (id=4237)
> patch for ports/xorg/xinit
> 
> Here is what I am thinking for this bug and bug 10598.	Can we use this
> instead, and revert the patch to xserver?

Looks good to me.  

Don't forget that this adds run2 to the dependencies for xinit
Comment 7 Yaakov Selkowitz 2009-09-30 20:29:28 UTC
(In reply to comment #6)
> Looks good to me.  
> 
> Don't forget that this adds run2 to the dependencies for xinit

That means we depend on both run and run2.  Unfortunately run2 is not a drop-in
replacement for run (run2 uses XML files instead of command-line arguments), so
we'll have to live with that for now.
Comment 8 Yaakov Selkowitz 2009-09-30 20:36:09 UTC
While I'm at it, I noticed that startxwin.sh (but not the .bat) contains:

rm -rf /tmp/.X11-unix

Doing that while another XWin display is running is a bad idea.  e.g. I use :1
for multiwindow, leaving :0 open for startx launching, and testing startxwin.sh
causes clients to be unable to connect to my :1 since the socket no longer
exists.  I'm going to change that to:

rm -f /tmp/.X11-unix/X0

Just like in the .bat, since we explicitly set DISPLAY=127.0.0.1:0.0 earlier in
the script.
Comment 9 Yaakov Selkowitz 2009-10-01 04:19:07 UTC
Shipped in xinit-1.1.1-5 and xorg-server-1.6.4-1; closing.
Comment 10 Jon TURNEY 2009-10-04 14:59:21 UTC
Created attachment 4253 [details]
Tidy ups for system.Xwinrc

Dropping this patch has dropped some tiding up in system.Xwinrc; here's a
respun patch with just that in it...

http://cygwin.com/ml/cygwin-xfree/2009-10/msg00023.html
Comment 11 Jon TURNEY 2009-10-04 21:22:44 UTC
Oh, and xterm started from the tray menu doesn't seem to inherit the correct
environment :-(
Comment 12 Jon TURNEY 2009-10-04 22:02:08 UTC
(In reply to comment #11)
> Oh, and xterm started from the tray menu doesn't seem to inherit the correct
> environment :-(

Hmm.. mysterious: comparing the output of running 'C:\cygwin\bin\run bash -l -c
"env | sort >/tmp/env"' in a DOS shell, and "env | sort >/tmp/Xenv" from
system.Xwinrc, the only significant difference seems to be PS1...
Comment 13 Yaakov Selkowitz 2009-10-07 01:33:24 UTC
(In reply to comment #10)
> Created an attachment (id=4253)
> Tidy ups for system.Xwinrc
> 
> Dropping this patch has dropped some tiding up in system.Xwinrc; here's a
> respun patch with just that in it...

FWIW, the default fd.o desktop menu entries launch xterm w/o any options.  I'm
not sure that we need to cater to anyone's preferences in system.XWinrc; users
should be customizing their ~/.XWinrc or ~/.Xdefaults to their tastes.

Patch, minus the '-sb' addition, added to the queue.

Comment 14 Jon TURNEY 2009-10-08 20:56:31 UTC
(In reply to comment #13)
> (In reply to comment #10)
> > Created an attachment (id=4253)
> > Tidy ups for system.Xwinrc
> > 
> > Dropping this patch has dropped some tiding up in system.Xwinrc; here's a
> > respun patch with just that in it...
> 
> FWIW, the default fd.o desktop menu entries launch xterm w/o any options.  I'm
> not sure that we need to cater to anyone's preferences in system.XWinrc; users
> should be customizing their ~/.XWinrc or ~/.Xdefaults to their tastes.
> 
> Patch, minus the '-sb' addition, added to the queue.

Yes, that's probably the right thing to do.  Thanks.

Comment 15 Jon TURNEY 2009-10-08 21:25:07 UTC
(In reply to comment #12)
> (In reply to comment #11)
> > Oh, and xterm started from the tray menu doesn't seem to inherit the correct
> > environment :-(
> 
> Hmm.. mysterious: comparing the output of running 'C:\cygwin\bin\run bash -l -c
> "env | sort >/tmp/env"' in a DOS shell, and "env | sort >/tmp/Xenv" from
> system.Xwinrc, the only significant difference seems to be PS1...

Learnt something new today: PS1 gets unset by non-interactive bash shells

http://git.savannah.gnu.org/cgit/bash.git/tree/shell.c, around line 600

So setting in in /etc/profile and trying to inherit it everywhere isn't going to
work :-(
Comment 16 Yaakov Selkowitz 2009-10-13 05:03:26 UTC
Patch shipped in 1.6.5-1; closing.