diff mbox series

[bug#64765] gnu: home: zsh: Also load enviroment in non-login shells

Message ID 20230721105801.10840-1-saku@laesvuori.fi
State New
Headers show
Series [bug#64765] gnu: home: zsh: Also load enviroment in non-login shells | expand

Commit Message

Saku Laesvuori July 21, 2023, 10:51 a.m. UTC
* gnu/home/services/shells.scm (zsh-file-zshenv): Add snippet to source
profiles.
(zsh-file-zprofile): Remove profile sourcing snippet.
(zsh-get-configuration-files): Always add .zshenv as it is never empty.
Check that .zprofile is not empty before adding it.
---
The service incorrectly assumed that shells are either login shells or
started from another shell. For example, ssh with a command argument
starts shells that aren't login shells nor started from another shell.

 gnu/home/services/shells.scm | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)


base-commit: e401eff97706dc6cdaf20b01dd12e291d7d13c2b

Comments

宋文武 July 21, 2023, 12:42 p.m. UTC | #1
Saku Laesvuori <saku@laesvuori.fi> writes:

> * gnu/home/services/shells.scm (zsh-file-zshenv): Add snippet to source
> profiles.
> (zsh-file-zprofile): Remove profile sourcing snippet.
> (zsh-get-configuration-files): Always add .zshenv as it is never empty.
> Check that .zprofile is not empty before adding it.
> ---
> The service incorrectly assumed that shells are either login shells or
> started from another shell. For example, ssh with a command argument
> starts shells that aren't login shells nor started from another shell.

Hello, this looks reasonable to me, only one question:
Will ~/.guix-home/profile/etc/profile be sourced multiple times with
duplicated search-path entries?  (eg: check 'env' in 'zsh' in 'zsh').
Saku Laesvuori July 21, 2023, 2:44 p.m. UTC | #2
> > * gnu/home/services/shells.scm (zsh-file-zshenv): Add snippet to source
> > profiles.
> > (zsh-file-zprofile): Remove profile sourcing snippet.
> > (zsh-get-configuration-files): Always add .zshenv as it is never empty.
> > Check that .zprofile is not empty before adding it.
> > ---
> > The service incorrectly assumed that shells are either login shells or
> > started from another shell. For example, ssh with a command argument
> > starts shells that aren't login shells nor started from another shell.
> 
> Hello, this looks reasonable to me, only one question:
> Will ~/.guix-home/profile/etc/profile be sourced multiple times with
> duplicated search-path entries?  (eg: check 'env' in 'zsh' in 'zsh').

Yes, but I don't think it causes any problems aside from adding useless
data to the environment. This could be prevented by exporting
GUIX_PROFILE_SOURCED=1 or something similar and only sourcing profiles
if it isn't set.
Ludovic Courtès Aug. 16, 2023, 8:48 p.m. UTC | #3
Hi,

Saku Laesvuori <saku@laesvuori.fi> skribis:

>> > * gnu/home/services/shells.scm (zsh-file-zshenv): Add snippet to source
>> > profiles.
>> > (zsh-file-zprofile): Remove profile sourcing snippet.
>> > (zsh-get-configuration-files): Always add .zshenv as it is never empty.
>> > Check that .zprofile is not empty before adding it.
>> > ---
>> > The service incorrectly assumed that shells are either login shells or
>> > started from another shell. For example, ssh with a command argument
>> > starts shells that aren't login shells nor started from another shell.
>> 
>> Hello, this looks reasonable to me, only one question:
>> Will ~/.guix-home/profile/etc/profile be sourced multiple times with
>> duplicated search-path entries?  (eg: check 'env' in 'zsh' in 'zsh').
>
> Yes, but I don't think it causes any problems aside from adding useless
> data to the environment. This could be prevented by exporting
> GUIX_PROFILE_SOURCED=1 or something similar and only sourcing profiles
> if it isn't set.

I doesn’t sound great though, and I’m sure it could break obscure
things, like #include_next in C.

Is there a way this could be avoided?  (I’m not familiar with Zsh so I’m
not offering to help; just looking for pending patches to apply.  :-))

Thanks,
Ludo’.
Saku Laesvuori Aug. 17, 2023, 7:36 a.m. UTC | #4
> >> Hello, this looks reasonable to me, only one question:
> >> Will ~/.guix-home/profile/etc/profile be sourced multiple times with
> >> duplicated search-path entries?  (eg: check 'env' in 'zsh' in 'zsh').
> >
> > Yes, but I don't think it causes any problems aside from adding useless
> > data to the environment. This could be prevented by exporting
> > GUIX_PROFILE_SOURCED=1 or something similar and only sourcing profiles
> > if it isn't set.
> 
> I doesn’t sound great though, and I’m sure it could break obscure
> things, like #include_next in C.
> 
> Is there a way this could be avoided?  (I’m not familiar with Zsh so I’m
> not offering to help; just looking for pending patches to apply.  :-))

It could be avoided properly by making guix-generated
profiles ensure that they can't be sourced multiple times
(e.g. [ -z "$GUIX_PROFILE_SOURCED" ] || return ; GUIX_PROFILE_SOURCED=1).
The specific problem I was facing was with running commands with ssh,
which the bash service seems to fix by sourcing /etc/profile from bashrc
when used via ssh. I'll send a patch for this ssh-specific solution but
I'm not sure if it's the best way to fix this.
diff mbox series

Patch

diff --git a/gnu/home/services/shells.scm b/gnu/home/services/shells.scm
index 7960590e7c..93a3b38267 100644
--- a/gnu/home/services/shells.scm
+++ b/gnu/home/services/shells.scm
@@ -182,21 +182,18 @@  (define* (zsh-field-not-empty? config field)
 (define (zsh-file-zshenv config)
   (mixed-text-file
    "zshenv"
-   (zsh-serialize-field config 'zshenv)
-   (zsh-serialize-field config 'environment-variables)))
-
-(define (zsh-file-zprofile config)
-  (mixed-text-file
-   "zprofile"
    "\
 # Set up the system, user profile, and related variables.
 source /etc/profile
 # Set up the home environment profile.
 source ~/.profile
-
-# It's only necessary if zsh is a login shell, otherwise profiles will
-# be already sourced by bash
 "
+   (zsh-serialize-field config 'zshenv)
+   (zsh-serialize-field config 'environment-variables)))
+
+(define (zsh-file-zprofile config)
+  (mixed-text-file
+   "zprofile"
    (zsh-serialize-field config 'zprofile)))
 
 (define (zsh-file-by-field config field)
@@ -208,10 +205,9 @@  (define (zsh-file-by-field config field)
         (zsh-serialize-field config field)))))
 
 (define (zsh-get-configuration-files config)
-  `((".zprofile" ,(zsh-file-by-field config 'zprofile)) ;; Always non-empty
-    ,@(if (or (zsh-field-not-empty? config 'zshenv)
-              (zsh-field-not-empty? config 'environment-variables))
-          `((".zshenv" ,(zsh-file-by-field config 'zshenv))) '())
+  `((".zshenv" ,(zsh-file-by-field config 'zshenv)) ;; Always non-empty
+    ,@(if (zsh-field-not-empty? config 'zprofile)
+          `((".zprofile" ,(zsh-file-by-field config 'zprofile))) '())
     ,@(if (zsh-field-not-empty? config 'zshrc)
           `((".zshrc" ,(zsh-file-by-field config 'zshrc))) '())
     ,@(if (zsh-field-not-empty? config 'zlogin)