nixos/shells: Avoid overriding the environment for other child shells #44720
Conversation
|
Yeah, I've seen your comments in the parent issue and I agree with the general direction.
I'll look over and test these changes in the next couple of days.
|
|
I don't like how complicated this gets, especially in the `bash` case. AFAICS all the cited issues are about simple env reset. Why do you treat login shells differently then?
For instance, in bash case, why can't you leave everything as it was, remove `${config.system.build.setEnvironment.text}` from `shellInit` and add
```
# Make sure we have a working environment
if [ -z "$__NIXOS_SET_ENVIRONMENT_DONE" ]; then
${config.system.build.setEnvironment.text}
fi
```
to `/etc/profile`? Am I missing something?
(Btw, I also think that this is a good time to rethink #30418. IMHO rc files that generates are pretty unreadable.)
|
Yeah, the bash code gets pretty ugly. The reasoning came from @rycee's comment. When thinking it through I agree that preserving the semantics probably isn't worth the complications. And I'm not even sure if login shells clearing some of the environment is a common semantic? If we're only sourcing the basic environment once then a common guard in
Wrapping a huge block in |
hedning
force-pushed
the
fix-shell-environment-clobber
branch
from
ca95a95
to
7e28f87
Compare
|
OK, pulled the whole guard into This is a lot simpler, and should work correctly for fish shell too I believe. |
|
Yep, now it nice and simple. However,
- The motivation behind @rycee's comment makes sense, providing a way to reset environment to the pristine one makes sense. I disagree with using login shells for that, though, as it makes things overly complicated.
- The motivation behind #30418 makes sense too, grepping /etc for environment is useful for newbies. I disagree with the implementation, though.
I think we can satisfy everyone by keeping the guard in the rc files (not in `setEnvironment`) and exposing `setEnvironment` as `/etc/environment` or something.
- Want manual reset? Source `/etc/environment`.
- Grepping /etc? It's there.
Thoughts?
|
|
Agreed, it would be good to have grepability and easy access to source the common environment again.
|
hedning
force-pushed
the
fix-shell-environment-clobber
branch
from
7e28f87
to
f89b198
Compare
hedning
force-pushed
the
fix-shell-environment-clobber
branch
from
f89b198
to
2732736
Compare
|
Went with |
|
I would call it `/etc/set-environment`, mainly for continuity with the old name, and since it's a script.
removing `system.build.setEnvironment`
Well, but having those files depend on each other and not on `/etc` has its merits. I wonder what @vcunat thinks about this (I vaguely remember you commented on something similar).
|
hedning
force-pushed
the
fix-shell-environment-clobber
branch
from
2732736
to
0cbd181
Compare
We move `system.build.setEnvironment` to `/etc/set-environment` making it easy to grep for environment variables in `/etc` and resetting the common environment if its wanted with `. /etc/set-environment`. A shared exported guard, `__NIXOS_SET_ENVIRONMENT_DONE`, is introduced that can be used to prevent child shells sourcing `/etc/set-environment` again if it's already done. This fixes eg. `nix run derivation` when run from eg. ZSH through the console or ssh. Before this Bash would resource the common environment resetting `PATH`.
hedning
force-pushed
the
fix-shell-environment-clobber
branch
from
0cbd181
to
fecb9ab
Compare
Good point.
Yeah, I'm fine with both. I guess referencing it from nix code means there's some checks at evaluation time at least. |
I can't recall anything similar right now :-) |
|
@oxij looks good 👍 , I'll just close this one. |
Motivation for this change
fixes #33219
All new ZSH and Bash shells source
system.build.setEnvironmentif it's not already done by a parent shell of the same type.For instance, if ZSH is the default shell, launching a Bash shell will reset any modifications to
PATH.One unfortunate effect of this behavior is
nix runlaunching a bash shell with an incorrectPATHwhen run from ZSH in eg. the console or SSH (for some reasonnix-shellisn't affected by this).The broken
nix runbehavior is a blocker for #44497, as the environment is set up using a login shell of the users choice, ie./etc/profileisn't sourced if using ZSH. In graphical sessions this was normally protected against by always sourcing/etc/profilecc @oxij as you've written a large part of the shell environment setup
Things done
This adds a shared exported guard,
__NIXOS_SET_ENVIRONMENT_DONE, that preventsinteractive non-login child shells of any type sourcing
system.build.setEnvironmentifit's already done.
Login shells on the other will always reset the basic environment.
I tried using
shopt -q login_shellto get the bash code to be more similar to ZSH, but that brokenixos.tests.login. Not that happy with the resulting flow of code inbash.nix, so if someone knows a more portable way to check for a login shell, that would be preferable I think.I've done some basic testing using both ZSH and Bash as default shells in a VM. Also tested
nixos.tests.loginandnixos.tests.env.I haven't looked Fish, but the guard should be set, so launching Bash from fish should work as expected.
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)