[PATCH] gnu: xterm: Accept $SHELL even if not in /etc/shells

[PATCH] gnu: xterm: Accept $SHELL even if not in /etc/shells

[Mark H Weaver]

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

This patch makes xterm honor $SHELL (or the shell in the user's password
entry) even if it's not in /etc/shells.  WDYT?

    Mark



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] gnu: xterm: Accept $SHELL even if not in /etc/shells --]
[-- Type: text/x-patch, Size: 3023 bytes --]

From 15d59a2d31794ffa6049bbcf878d568593099dc5 Mon Sep 17 00:00:00 2001
From: Mark H Weaver <mhw@netris.org>
Date: Thu, 13 Feb 2014 02:00:39 -0500
Subject: [PATCH] gnu: xterm: Accept $SHELL even if not in /etc/shells.

* gnu/packages/patches/xterm-shell.patch: New file.
* gnu/packages/xorg.scm (xterm): Add the patch.
* gnu-system.am (dist_patch_DATA): Add the patch.
---
 gnu-system.am                          |  3 ++-
 gnu/packages/patches/xterm-shell.patch | 22 ++++++++++++++++++++++
 gnu/packages/xorg.scm                  |  3 ++-
 3 files changed, 26 insertions(+), 2 deletions(-)
 create mode 100644 gnu/packages/patches/xterm-shell.patch

diff --git a/gnu-system.am b/gnu-system.am
index 7110a60..66ff0f1 100644
--- a/gnu-system.am
+++ b/gnu-system.am
@@ -310,7 +310,8 @@ dist_patch_DATA =						\
   gnu/packages/patches/vpnc-script.patch			\
   gnu/packages/patches/w3m-fix-compile.patch			\
   gnu/packages/patches/xmodmap-asprintf.patch			\
-  gnu/packages/patches/xpdf-constchar.patch
+  gnu/packages/patches/xpdf-constchar.patch			\
+  gnu/packages/patches/xterm-shell.patch
 
 bootstrapdir = $(guilemoduledir)/gnu/packages/bootstrap
 bootstrap_x86_64_linuxdir = $(bootstrapdir)/x86_64-linux
diff --git a/gnu/packages/patches/xterm-shell.patch b/gnu/packages/patches/xterm-shell.patch
new file mode 100644
index 0000000..c3f65d3
--- /dev/null
+++ b/gnu/packages/patches/xterm-shell.patch
@@ -0,0 +1,22 @@
+Accept the value of $SHELL or the shell in the user's password entry,
+even if it's not found in /etc/shells.
+
+Patch by Mark H Weaver <mhw@netris.org>.
+
+--- xterm/main.c.orig	2014-01-15 21:12:25.000000000 -0500
++++ xterm/main.c	2014-02-13 01:55:04.840576171 -0500
+@@ -4570,12 +4570,12 @@
+ 	    if (validShell(explicit_shname)) {
+ 		xtermSetenv("SHELL", explicit_shname);
+ 		shell_path = explicit_shname;
+-	    } else if (validShell(shell_path = x_getenv("SHELL"))) {
++	    } else if (shell_path = x_getenv("SHELL")) {
+ 		;		/* OK */
+ 	    } else if ((!OkPasswd(&pw) && !x_getpwuid(screen->uid, &pw))
+ 		       || *(shell_path = x_strdup(pw.pw_shell)) == 0) {
+ 		shell_path = resetShell(shell_path);
+-	    } else if (validShell(shell_path)) {
++	    } else if (x_nonempty(shell_path)) {
+ 		xtermSetenv("SHELL", shell_path);
+ 	    } else {
+ 		shell_path = resetShell(shell_path);
diff --git a/gnu/packages/xorg.scm b/gnu/packages/xorg.scm
index abcbfba..85df02f 100644
--- a/gnu/packages/xorg.scm
+++ b/gnu/packages/xorg.scm
@@ -4721,7 +4721,8 @@ icccm: Both client and window-manager helpers for ICCCM.")
                "http://invisible-island.net/datafiles/release/xterm.tar.gz")
               (sha256
                (base32
-                "040rarvv18zg0lk7qy0m3n7gv10mh40jic708wvng01z4rlbpfhz"))))
+                "040rarvv18zg0lk7qy0m3n7gv10mh40jic708wvng01z4rlbpfhz"))
+              (patches (list (search-patch "xterm-shell.patch")))))
     (build-system gnu-build-system)
     (arguments
      '(#:configure-flags '("--enable-wide-chars" "--enable-256-color"
-- 
1.8.4

Re: [PATCH] gnu: xterm: Accept $SHELL even if not in /etc/shells

[John Darrington]

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

On Thu, Feb 13, 2014 at 03:00:29AM -0500, Mark H Weaver wrote:
     This patch makes xterm honor $SHELL (or the shell in the user's password
     entry) even if it's not in /etc/shells.  WDYT?
     

It sounds like a good idea to me.  /etc/shells is supposed to be only a whitelist of
those shells which may be used for login.  Not an exhaustive list of shells which
may be used at all.

However, I'm wondering if we are forking too many upstream packages.  We should
only patch software in order to allow it to build/install.  If we want to change
the behaviour like here, that should be sent to upstream for their next release.

If they refuse the patch, then of course you can start your own weavershell fork...

J'


-- 
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] gnu: xterm: Accept $SHELL even if not in /etc/shells

[Mark H Weaver]

John Darrington <john@darrington.wattle.id.au> writes:

> On Thu, Feb 13, 2014 at 03:00:29AM -0500, Mark H Weaver wrote:
>      This patch makes xterm honor $SHELL (or the shell in the user's password
>      entry) even if it's not in /etc/shells.  WDYT?
>      
>
> It sounds like a good idea to me.  /etc/shells is supposed to be only a whitelist of
> those shells which may be used for login.  Not an exhaustive list of shells which
> may be used at all.
>
> However, I'm wondering if we are forking too many upstream packages.  We should
> only patch software in order to allow it to build/install.

Really?  Just enough to build/install?  Not enough to work properly?  I
agree that we should stay as close as we reasonably can to upstream, but
sometimes things have to be fixed to work with Guix, which after all is
a rather unusual distro.

FYI, xterm doesn't merely ignore your $SHELL setting if it's not in
/etc/shells, it also *sets* $SHELL to "/bin/sh" for you in that case,
and then proceeds runs it.

IMO, it's not reasonable to have to add
/home/<USER>/<PROFILE>/bin/<SHELL> for every combination of <USER>,
<PROFILE>, and <SHELL> to /etc/shells, in order to prevent 'xterm' from
overriding your $SHELL setting.

> If we want to change the behaviour like here, that should be sent to
> upstream for their next release.

Well, I agree that it would be good to try this, however:

> If they refuse the patch, then of course you can start your own
> weavershell fork...

Fork it to change two lines?

      Mark

Re: [PATCH] gnu: xterm: Accept $SHELL even if not in /etc/shells

[John Darrington]

On Thu, Feb 13, 2014 at 03:47:54AM -0500, Mark H Weaver wrote:
     John Darrington <john@darrington.wattle.id.au> writes:
     
     > On Thu, Feb 13, 2014 at 03:00:29AM -0500, Mark H Weaver wrote:
     >      This patch makes xterm honor $SHELL (or the shell in the user's password
     >      entry) even if it's not in /etc/shells.  WDYT?
     >      
     > It sounds like a good idea to me.  /etc/shells is supposed to be only a whitelist of
     > those shells which may be used for login.  Not an exhaustive list of shells which
     > may be used at all.
     >
     > However, I'm wondering if we are forking too many upstream packages.  We should
     > only patch software in order to allow it to build/install.
     
     Really?  Just enough to build/install?  Not enough to work properly?  I
     agree that we should stay as close as we reasonably can to upstream, but
     sometimes things have to be fixed to work with Guix, which after all is
     a rather unusual distro.

     FYI, xterm doesn't merely ignore your $SHELL setting if it's not in
     /etc/shells, it also *sets* $SHELL to "/bin/sh" for you in that case,
     and then proceeds runs it.

Then that to me, sounds like a bug in xterm and can potentially affect many OSes not only guix.
     
     IMO, it's not reasonable to have to add
     /home/<USER>/<PROFILE>/bin/<SHELL> for every combination of <USER>,
     <PROFILE>, and <SHELL> to /etc/shells, in order to prevent 'xterm' from
     overriding your $SHELL setting.

I don't disagree.
     
     > If they refuse the patch, then of course you can start your own
     > weavershell fork...
     
     Fork it to change two lines?
     
Two lines today, five tomorrow, twenty next week ...

It is true that Guix is somewhat unusual - and therefore it exposes bugs in 
packages which hitherto have gone unnoticed.  That doesn't change the fact that 
they are bugs in upstream.  Of course it might be difficult getting upstream to
accept a patch but we should try.

I'm just making the point that Guix is not a repository of bug fixes!
     
Just my $0.02

J'


-- 
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Re: [PATCH] gnu: xterm: Accept $SHELL even if not in /etc/shells

[Ludovic Courtès]

Mark H Weaver <mhw@netris.org> skribis:

> IMO, it's not reasonable to have to add
> /home/<USER>/<PROFILE>/bin/<SHELL> for every combination of <USER>,
> <PROFILE>, and <SHELL> to /etc/shells, in order to prevent 'xterm' from
> overriding your $SHELL setting.

On NixOS, /etc/shells contains this:

--8<---------------cut here---------------start------------->8---
/run/current-system/sw/bin/bash
/var/run/current-system/sw/bin/bash
/bin/sh
--8<---------------cut here---------------end--------------->8---

Where {/var/,}/run/current-system contains the “global” profile, like on
our QEMU images.

Perhaps that’s good enough no?

(As I see it, the stand-alone GNU system will have /bin/sh (as a
symlink) and /run/current-system too.)

Ludo’.

Re: [PATCH] gnu: xterm: Accept $SHELL even if not in /etc/shells

[Mark H Weaver]

ludo@gnu.org (Ludovic Courtès) writes:

> Mark H Weaver <mhw@netris.org> skribis:
>
>> IMO, it's not reasonable to have to add
>> /home/<USER>/<PROFILE>/bin/<SHELL> for every combination of <USER>,
>> <PROFILE>, and <SHELL> to /etc/shells, in order to prevent 'xterm' from
>> overriding your $SHELL setting.
>
> On NixOS, /etc/shells contains this:
>
> /run/current-system/sw/bin/bash
> /var/run/current-system/sw/bin/bash
> /bin/sh
>
> Where {/var/,}/run/current-system contains the “global” profile, like on
> our QEMU images.
>
> Perhaps that’s good enough no?

If a user wants to set $SHELL to be the one in their private profile,
I think 'xterm' shouldn't ignore it and modify $SHELL just because it
hasn't been authorized by the administrator of the system.  This seems
to me to be against the spirit of "liberating".

Well, it seems that I'll have to live on a local branch of Guix, with my
own modifications.  I suppose this is not necessarily a bad thing.  The
ease with which this can be done is part of what makes Guix liberating,
after all.

     Mark

Re: [PATCH] gnu: xterm: Accept $SHELL even if not in /etc/shells

[Ludovic Courtès]

Mark H Weaver <mhw@netris.org> skribis:

> ludo@gnu.org (Ludovic Courtès) writes:
>
>> Mark H Weaver <mhw@netris.org> skribis:
>>
>>> IMO, it's not reasonable to have to add
>>> /home/<USER>/<PROFILE>/bin/<SHELL> for every combination of <USER>,
>>> <PROFILE>, and <SHELL> to /etc/shells, in order to prevent 'xterm' from
>>> overriding your $SHELL setting.
>>
>> On NixOS, /etc/shells contains this:
>>
>> /run/current-system/sw/bin/bash
>> /var/run/current-system/sw/bin/bash
>> /bin/sh
>>
>> Where {/var/,}/run/current-system contains the “global” profile, like on
>> our QEMU images.
>>
>> Perhaps that’s good enough no?
>
> If a user wants to set $SHELL to be the one in their private profile,
> I think 'xterm' shouldn't ignore it and modify $SHELL just because it
> hasn't been authorized by the administrator of the system.

Agreed.

However, we’re just packaging an existing application.  IMO, when we
find such limitations (it’s really a limitation, and not something that
makes it completely unusable), we should submit the improvement
upstream, unless upstream no longer exists (I’m not sure if this is the
case here.)

WDYT?

Thanks!

Ludo’.

Re: [PATCH] gnu: xterm: Accept $SHELL even if not in /etc/shells

[John Darrington]

On Fri, Feb 14, 2014 at 11:59:32AM +0100, Ludovic Court??s wrote:
     However, we???re just packaging an existing application.  IMO, when we
     find such limitations (it???s really a limitation, and not something that
     makes it completely unusable), we should submit the improvement
     upstream, unless upstream no longer exists (I???m not sure if this is the
     case here.)
     

I think the following are true (please correct me if not):

* Most (all?) the files in gnu/packages/patches fall into the category of 
  "limitations" to upstream.

* In principle, those patch files could be directly applied to upstream
  without modification.


This being the case, would it not be a good idea, to have some kind of web
interface to these patch files to make it easy for upstream maintainers to 
fetch them.  (perhaps there is sucha page already) Then we can publish this
page and say "hey upstream maintainer! Your package has limitations.
Please apply these patches."

J'

-- 
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Re: [PATCH] gnu: xterm: Accept $SHELL even if not in /etc/shells

[Alex Sassmannshausen]

Hello,

Not the most experienced "upstream maintainer", but …


John Darrington writes:

> This being the case, would it not be a good idea, to have some kind of web
> interface to these patch files to make it easy for upstream maintainers to 
> fetch them.  (perhaps there is sucha page already) Then we can publish this
> page and say "hey upstream maintainer! Your package has limitations.
> Please apply these patches."

This sounds like a pretty nifty idea!

Alex

Re: [PATCH] gnu: xterm: Accept $SHELL even if not in /etc/shells

[Ludovic Courtès]

John Darrington <john@darrington.wattle.id.au> skribis:

> On Fri, Feb 14, 2014 at 11:59:32AM +0100, Ludovic Court??s wrote:
>      However, we???re just packaging an existing application.  IMO, when we
>      find such limitations (it???s really a limitation, and not something that
>      makes it completely unusable), we should submit the improvement
>      upstream, unless upstream no longer exists (I???m not sure if this is the
>      case here.)
>      
>
> I think the following are true (please correct me if not):
>
> * Most (all?) the files in gnu/packages/patches fall into the category of 
>   "limitations" to upstream.
>
> * In principle, those patch files could be directly applied to upstream
>   without modification.

I think most of the patches in there are fixes (normally submitted
upstream), or adjustments so that things can build/run in our
environment (patches we don’t want to submit.)

> This being the case, would it not be a good idea, to have some kind of web
> interface to these patch files to make it easy for upstream maintainers to 
> fetch them.  (perhaps there is sucha page already)

Yes, the page is:

  http://www.gnu.org/software/guix/package-list.html

It lists patches and ‘patch snippets’ for each package.

Ludo’.