[bug#60365] [PATCH 0/4] Rationalize 'PS1' handling in home, system, and shell

[bug#60365] [PATCH 0/4] Rationalize 'PS1' handling in home, system, and shell

[Ludovic Courtès]

Hello!

I found some things could be improved in how we deal with ‘PS1’ in
Guix Home, Guix System, and ‘guix shell’ (my initial motivation was
making it easy to change ‘PS1’ from a Home configuration).

These patches fiddle with ‘PS1’.  The most visible effect is that
Guix Home’s ~/.bashrc will no longer provide a default ‘PS1’.

Thoughts?

Ludo’.

Ludovic Courtès (4):
  system, home: Factorize default '.bashrc'.
  system: Define default 'PS1' in /etc/bashrc rather than ~/.bashrc.
  environment: Simplify 'PS1' suggestion on '--check'.
  environment: Avoid false positive on 'PS1' check.

 gnu/home/services/shells.scm | 41 +++---------------------------------
 gnu/system.scm               |  3 +++
 gnu/system/shadow.scm        | 33 +++++++++++++----------------
 guix/scripts/environment.scm | 12 ++++++-----
 4 files changed, 28 insertions(+), 61 deletions(-)


base-commit: 9369c1ccf47d9bf6f2e28a9454c1c329a2044f19
prerequisite-patch-id: aae3af39aa7753dd7ed6d0584587a06d51402386
-- 
2.38.1

[bug#60365] [PATCH 1/4] system, home: Factorize default '.bashrc'.

[Ludovic Courtès]

* gnu/system/shadow.scm (%default-bashrc): New variable.  Source
/etc/bashrc only if it exists.
(default-skeletons): Use it.
* gnu/home/services/shells.scm (guix-bashrc): Remove.
(add-bash-configuration): Refer to '%default-bashrc' instead.
---
 gnu/home/services/shells.scm | 41 +++---------------------------------
 gnu/system/shadow.scm        | 26 +++++++++++++----------
 2 files changed, 18 insertions(+), 49 deletions(-)

diff --git a/gnu/home/services/shells.scm b/gnu/home/services/shells.scm
index b529c8e798..d7dd579293 100644
--- a/gnu/home/services/shells.scm
+++ b/gnu/home/services/shells.scm
@@ -19,6 +19,7 @@
 
 (define-module (gnu home services shells)
   #:use-module (gnu services configuration)
+  #:autoload   (gnu system shadow) (%default-bashrc)
   #:use-module (gnu home services utils)
   #:use-module (gnu home services)
   #:use-module (gnu packages shells)
@@ -370,43 +371,6 @@ (define-configuration home-bash-configuration
 won't be read in some cases (if the shell terminates by exec'ing
 another process for example)."))
 
-;; TODO: Use value from (gnu system shadow)
-(define guix-bashrc
-  "\
-# Bash initialization for interactive non-login shells and
-# for remote shells (info \"(bash) Bash Startup Files\").
-
-# Export 'SHELL' to child processes.  Programs such as 'screen'
-# honor it and otherwise use /bin/sh.
-export SHELL
-
-if [[ $- != *i* ]]
-then
-    # We are being invoked from a non-interactive shell.  If this
-    # is an SSH session (as in \"ssh host command\"), source
-    # /etc/profile so we get PATH and other essential variables.
-    [[ -n \"$SSH_CLIENT\" ]] && source /etc/profile
-
-    # Don't do anything else.
-    return
-fi
-
-# Source the system-wide file.
-if [[ -e /etc/bashrc ]]; then
-    source /etc/bashrc
-fi
-
-# Adjust the prompt depending on whether we're in 'guix environment'.
-if [ -n \"$GUIX_ENVIRONMENT\" ]
-then
-    PS1='\\u@\\h \\w [env]\\$ '
-else
-    PS1='\\u@\\h \\w\\$ '
-fi
-alias ls='ls -p --color=auto'
-alias ll='ls -l'
-alias grep='grep --color=auto'\n")
-
 (define (add-bash-configuration config)
   (define (filter-fields field)
     (filter-configuration-fields home-bash-configuration-fields
@@ -449,7 +413,8 @@ (define* (file-if-not-empty field #:optional (extra-content #f))
      ,@(list (file-if-not-empty
               'bashrc
               (if (home-bash-configuration-guix-defaults? config)
-                  (list (serialize-field 'aliases) guix-bashrc)
+                  (list (serialize-field 'aliases)
+                        (plain-file-content %default-bashrc))
                   (list (serialize-field 'aliases))))
              (file-if-not-empty 'bash-logout)))))
 
diff --git a/gnu/system/shadow.scm b/gnu/system/shadow.scm
index 59f0a02c8b..4360a5ebd1 100644
--- a/gnu/system/shadow.scm
+++ b/gnu/system/shadow.scm
@@ -1,5 +1,5 @@
 ;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2013-2020, 2022 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2016 Alex Griffin <a@ajgrf.com>
 ;;; Copyright © 2020 Jan (janneke) Nieuwenhuizen <janneke@gnu.org>
 ;;; Copyright © 2020 Efraim Flashner <efraim@flashner.co.il>
@@ -63,7 +63,8 @@ (define-module (gnu system shadow)
                user-group-id
                user-group-system?)
 
-  #:export (default-skeletons
+  #:export (%default-bashrc
+            default-skeletons
             skeleton-directory
             %base-groups
             %base-user-accounts
@@ -118,14 +119,8 @@ (define %base-user-accounts
          (create-home-directory? #f)
          (system? #t))))
 
-(define (default-skeletons)
-  "Return the default skeleton files for /etc/skel.  These files are copied by
-'useradd' in the home directory of newly created user accounts."
-
-  (let ((profile (plain-file "bash_profile" "\
-# Honor per-interactive-shell startup file
-if [ -f ~/.bashrc ]; then . ~/.bashrc; fi\n"))
-        (bashrc  (plain-file "bashrc" "\
+(define %default-bashrc
+  (plain-file "bashrc" "\
 # Bash initialization for interactive non-login shells and
 # for remote shells (info \"(bash) Bash Startup Files\").
 
@@ -145,7 +140,7 @@ (define (default-skeletons)
 fi
 
 # Source the system-wide file.
-source /etc/bashrc
+[ -f /etc/bashrc ] && source /etc/bashrc
 
 # Adjust the prompt depending on whether we're in 'guix environment'.
 if [ -n \"$GUIX_ENVIRONMENT\" ]
@@ -157,6 +152,15 @@ (define (default-skeletons)
 alias ls='ls -p --color=auto'
 alias ll='ls -l'
 alias grep='grep --color=auto'\n"))
+
+(define (default-skeletons)
+  "Return the default skeleton files for /etc/skel.  These files are copied by
+'useradd' in the home directory of newly created user accounts."
+
+  (let ((profile (plain-file "bash_profile" "\
+# Honor per-interactive-shell startup file
+if [ -f ~/.bashrc ]; then . ~/.bashrc; fi\n"))
+        (bashrc  %default-bashrc)
         (zprofile    (plain-file "zprofile" "\
 # Honor system-wide environment variables
 source /etc/profile\n"))
-- 
2.38.1

[bug#60365] [PATCH 2/4] system: Define default 'PS1' in /etc/bashrc rather than ~/.bashrc.

[Ludovic Courtès]

Users can override 'PS1' in ~/.bashrc if they wish.

Previously, on Guix Home, the "default" 'PS1' would be set in ~/.bashrc
when 'home-bash-configuration-guix-defaults?' is true, preventing users
from overriding it via the 'environment-variables' field of
'home-bash-extension'.

* gnu/system/shadow.scm (%default-bashrc): Remove 'PS1' setting.
* gnu/system.scm (operating-system-etc-service): Define PS1 in
/etc/bashrc.
---
 gnu/system.scm        | 3 +++
 gnu/system/shadow.scm | 7 -------
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/gnu/system.scm b/gnu/system.scm
index 62c8e0c2b6..d67f9a615b 100644
--- a/gnu/system.scm
+++ b/gnu/system.scm
@@ -1047,6 +1047,9 @@ (define* (operating-system-etc-service os)
         (bashrc    (plain-file "bashrc" "\
 # Bash-specific initialization.
 
+# Provide a default prompt.  The user's ~/.bashrc can override it.
+PS1='\\u@\\h \\w${GUIX_ENVIRONMENT:+ [env]}\\$ '
+
 # The 'bash-completion' package.
 if [ -f /run/current-system/profile/etc/profile.d/bash_completion.sh ]
 then
diff --git a/gnu/system/shadow.scm b/gnu/system/shadow.scm
index 4360a5ebd1..2e87928368 100644
--- a/gnu/system/shadow.scm
+++ b/gnu/system/shadow.scm
@@ -142,13 +142,6 @@ (define %default-bashrc
 # Source the system-wide file.
 [ -f /etc/bashrc ] && source /etc/bashrc
 
-# Adjust the prompt depending on whether we're in 'guix environment'.
-if [ -n \"$GUIX_ENVIRONMENT\" ]
-then
-    PS1='\\u@\\h \\w [env]\\$ '
-else
-    PS1='\\u@\\h \\w\\$ '
-fi
 alias ls='ls -p --color=auto'
 alias ll='ls -l'
 alias grep='grep --color=auto'\n"))
-- 
2.38.1

[bug#60365] [PATCH 3/4] environment: Simplify 'PS1' suggestion on '--check'.

[Ludovic Courtès]

* guix/scripts/environment.scm (validate-child-shell-environment): In
'PS1' suggestion, remove 'export' and avoid 'if'.
---
 guix/scripts/environment.scm | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/guix/scripts/environment.scm b/guix/scripts/environment.scm
index ab11b35335..bdbfa03fcf 100644
--- a/guix/scripts/environment.scm
+++ b/guix/scripts/environment.scm
@@ -610,10 +610,7 @@ (define-syntax-rule (warn exp ...)
 @file{~/.bashrc}:
 
 @example
-if [ -n \"$GUIX_ENVIRONMENT\" ]
-then
-  export PS1=\"\\u@@\\h \\w [env]\\$ \"
-fi
+PS1='\\u@@\\h \\w${GUIX_ENVIRONMENT:+ [env]}\\$ '
 @end example
 "))))))
 
-- 
2.38.1

[bug#60365] [PATCH 4/4] environment: Avoid false positive on 'PS1' check.

[Ludovic Courtès]

* guix/scripts/environment.scm (validate-child-shell-environment): Do
not warn when 'PS1' refers to 'GUIX_ENVIRONMENT'.
---
 guix/scripts/environment.scm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/guix/scripts/environment.scm b/guix/scripts/environment.scm
index bdbfa03fcf..c7fd8fd340 100644
--- a/guix/scripts/environment.scm
+++ b/guix/scripts/environment.scm
@@ -601,7 +601,12 @@ (define-syntax-rule (warn exp ...)
       (match (vhash-assoc "PS1" actual)
         (#f #f)
         ((_ . str)
-         (when (and (getenv "PS1") (string=? str (getenv "PS1")))
+         (when (and (getenv "PS1") (string=? str (getenv "PS1"))
+
+                    ;; 'PS1' might be conditional on 'GUIX_ENVIRONMENT', as
+                    ;; shown in the hint below.
+                    (not (or (string-contains str "$GUIX_ENVIRONMENT")
+                             (string-contains str "${GUIX_ENVIRONMENT"))))
            (warning (G_ "'PS1' is the same in sub-shell~%"))
            (display-hint (G_ "Consider setting a different prompt for
 environment shells to make them distinguishable.
-- 
2.38.1

[bug#60365] [PATCH 0/4] Rationalize 'PS1' handling in home, system, and shell

[Andrew Tropin]

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

On 2022-12-27 22:23, Ludovic Courtès wrote:

> Hello!
>

Hi Ludovic!

> I found some things could be improved in how we deal with ‘PS1’ in
> Guix Home, Guix System, and ‘guix shell’ (my initial motivation was
> making it easy to change ‘PS1’ from a Home configuration).
>
> These patches fiddle with ‘PS1’.  The most visible effect is that
> Guix Home’s ~/.bashrc will no longer provide a default ‘PS1’.
>
> Thoughts?

Factorization of bashrc is great!

Not sure about moving PS1 to /etc/bashrc as it won't work on foreign
distros and guix environment won't be noticable in the prompt.  Warning
is useful, but I would expect prompt work OOB, when guix-defualts? is
#t.

The possible alternative solution is to factorize bash_profile and put
PS1 here, so both skeleton and home provided bashrc have it, after that
setting PS1 with environment-variables field will start working.

>
> Ludo’.
>
> Ludovic Courtès (4):
>   system, home: Factorize default '.bashrc'.
>   system: Define default 'PS1' in /etc/bashrc rather than ~/.bashrc.
>   environment: Simplify 'PS1' suggestion on '--check'.
>   environment: Avoid false positive on 'PS1' check.
>
>  gnu/home/services/shells.scm | 41 +++---------------------------------
>  gnu/system.scm               |  3 +++
>  gnu/system/shadow.scm        | 33 +++++++++++++----------------
>  guix/scripts/environment.scm | 12 ++++++-----
>  4 files changed, 28 insertions(+), 61 deletions(-)
>
>
> base-commit: 9369c1ccf47d9bf6f2e28a9454c1c329a2044f19
> prerequisite-patch-id: aae3af39aa7753dd7ed6d0584587a06d51402386

-- 
Best regards,
Andrew Tropin

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

[bug#60365] [PATCH 0/4] Rationalize 'PS1' handling in home, system, and shell

[Ludovic Courtès]

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

Hi Andrew!

Andrew Tropin <andrew@trop.in> skribis:

>> I found some things could be improved in how we deal with ‘PS1’ in
>> Guix Home, Guix System, and ‘guix shell’ (my initial motivation was
>> making it easy to change ‘PS1’ from a Home configuration).
>>
>> These patches fiddle with ‘PS1’.  The most visible effect is that
>> Guix Home’s ~/.bashrc will no longer provide a default ‘PS1’.
>>
>> Thoughts?
>
> Factorization of bashrc is great!
>
> Not sure about moving PS1 to /etc/bashrc as it won't work on foreign
> distros and guix environment won't be noticable in the prompt.  Warning
> is useful, but I would expect prompt work OOB, when guix-defualts? is
> #t.

Yes, you’re right.

> The possible alternative solution is to factorize bash_profile and put
> PS1 here, so both skeleton and home provided bashrc have it, after that
> setting PS1 with environment-variables field will start working.

How about this:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 564 bytes --]

diff --git a/gnu/home/services/shells.scm b/gnu/home/services/shells.scm
index d7dd579293..8d7db6b774 100644
--- a/gnu/home/services/shells.scm
+++ b/gnu/home/services/shells.scm
@@ -407,6 +407,9 @@ (define* (file-if-not-empty field #:optional (extra-content #f))
 # Honor per-interactive-shell startup file
 if [ -f ~/.bashrc ]; then source ~/.bashrc; fi
 "
+        (if (home-bash-configuration-guix-defaults? config)
+            "export PS1=...\n"
+            "")
         (serialize-field 'bash-profile)
         (serialize-field 'environment-variables)))
 

[-- Attachment #3: Type: text/plain, Size: 150 bytes --]


That way, if one adds ‘PS1’ to ‘environment-variables’, that will
override the default.

WDYT?

Thanks for your feedback!

Ludo’.

[bug#60365] [PATCH 0/4] Rationalize 'PS1' handling in home, system, and shell

[Andrew Tropin]

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

On 2023-01-03 23:46, Ludovic Courtès wrote:

> Hi Andrew!
>
> Andrew Tropin <andrew@trop.in> skribis:
>
>>> I found some things could be improved in how we deal with ‘PS1’ in
>>> Guix Home, Guix System, and ‘guix shell’ (my initial motivation was
>>> making it easy to change ‘PS1’ from a Home configuration).
>>>
>>> These patches fiddle with ‘PS1’.  The most visible effect is that
>>> Guix Home’s ~/.bashrc will no longer provide a default ‘PS1’.
>>>
>>> Thoughts?
>>
>> Factorization of bashrc is great!
>>
>> Not sure about moving PS1 to /etc/bashrc as it won't work on foreign
>> distros and guix environment won't be noticable in the prompt.  Warning
>> is useful, but I would expect prompt work OOB, when guix-defualts? is
>> #t.
>
> Yes, you’re right.
>
>> The possible alternative solution is to factorize bash_profile and put
>> PS1 here, so both skeleton and home provided bashrc have it, after that
>> setting PS1 with environment-variables field will start working.
>
> How about this:
>
> diff --git a/gnu/home/services/shells.scm b/gnu/home/services/shells.scm
> index d7dd579293..8d7db6b774 100644
> --- a/gnu/home/services/shells.scm
> +++ b/gnu/home/services/shells.scm
> @@ -407,6 +407,9 @@ (define* (file-if-not-empty field #:optional (extra-content #f))
>  # Honor per-interactive-shell startup file
>  if [ -f ~/.bashrc ]; then source ~/.bashrc; fi
>  "
> +        (if (home-bash-configuration-guix-defaults? config)
> +            "export PS1=...\n"

Yep, something like that should work.  Ideally, is to make
%default-bash-profile or even more generic %default-profile and reuse it
across system and home, but it may be not that trivial, so we can
postpone it for now.

> +            "")
>          (serialize-field 'bash-profile)
>          (serialize-field 'environment-variables)))
>  
>
> That way, if one adds ‘PS1’ to ‘environment-variables’, that will
> override the default.
>
> WDYT?
>
> Thanks for your feedback!
>
> Ludo’.

-- 
Best regards,
Andrew Tropin

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

bug#60365: [PATCH 0/4] Rationalize 'PS1' handling in home, system, and shell

[Ludovic Courtès]

Andrew Tropin <andrew@trop.in> skribis:

> On 2023-01-03 23:46, Ludovic Courtès wrote:

[...]

>>> Not sure about moving PS1 to /etc/bashrc as it won't work on foreign
>>> distros and guix environment won't be noticable in the prompt.  Warning
>>> is useful, but I would expect prompt work OOB, when guix-defualts? is
>>> #t.
>>
>> Yes, you’re right.
>>
>>> The possible alternative solution is to factorize bash_profile and put
>>> PS1 here, so both skeleton and home provided bashrc have it, after that
>>> setting PS1 with environment-variables field will start working.
>>
>> How about this:
>>
>> diff --git a/gnu/home/services/shells.scm b/gnu/home/services/shells.scm
>> index d7dd579293..8d7db6b774 100644
>> --- a/gnu/home/services/shells.scm
>> +++ b/gnu/home/services/shells.scm
>> @@ -407,6 +407,9 @@ (define* (file-if-not-empty field #:optional (extra-content #f))
>>  # Honor per-interactive-shell startup file
>>  if [ -f ~/.bashrc ]; then source ~/.bashrc; fi
>>  "
>> +        (if (home-bash-configuration-guix-defaults? config)
>> +            "export PS1=...\n"
>
> Yep, something like that should work.  Ideally, is to make
> %default-bash-profile or even more generic %default-profile and reuse it
> across system and home, but it may be not that trivial, so we can
> postpone it for now.

I adjusted things as proposed and pushed:

  b5e3701f1a environment: Avoid false positive on 'PS1' check.
  632e2f7ce1 environment: Simplify 'PS1' suggestion on '--check'.
  086df6ef21 system: Define default 'PS1' in /etc/bashrc rather than ~/.bashrc.
  2127e56bd8 system, home: Factorize default '.bashrc'.

Thanks!

Ludo’.